mirror of
https://github.com/open-policy-agent/opa.git
synced 2026-08-14 04:12:41 -06:00
Remove input checks in query compiler
Previously the query compiler checked whether input was defined to catch two cases: 1) Input was required by query or transitive dependencies of query. Input was required if the input document was referenced at all. Motivation for this was to produce "correct" results when negation is used. In practice, this never proved to be very helpful. 2) Input document was specified multiple times, causing a conflict. Again, in practice, this never proved to be very helpful. In both cases, if the policy decision is *incorrect* someone has to look at (i) the input (ii) the data and (iii) the policy to understand why. Ultimately we expect users to push schema information into OPA so that we can validate inputs and data conform to those schema. In that case, if an input was not specified (or conflicting); the type checking should catch it.
This commit is contained in:
@@ -690,7 +690,6 @@ func (qc *queryCompiler) Compile(query Body) (Body, error) {
|
||||
qc.resolveRefs,
|
||||
qc.checkWithModifiers,
|
||||
qc.checkSafety,
|
||||
qc.checkInput,
|
||||
qc.checkTypes,
|
||||
}
|
||||
|
||||
@@ -742,73 +741,6 @@ func (qc *queryCompiler) checkSafety(_ *QueryContext, body Body) (Body, error) {
|
||||
return reordered, nil
|
||||
}
|
||||
|
||||
func (qc *queryCompiler) checkInput(qctx *QueryContext, body Body) (Body, error) {
|
||||
return body, qc.checkInputRec(qctx.InputDefined(), body)
|
||||
}
|
||||
|
||||
func (qc *queryCompiler) checkInputRec(definedPrev bool, body Body) error {
|
||||
|
||||
// Perform DFS for conflicting or missing input document.
|
||||
for _, expr := range body {
|
||||
|
||||
definedCurr := definesInput(expr)
|
||||
|
||||
if definedPrev && definedCurr {
|
||||
return NewError(InputErr, expr.Location, "input document conflict")
|
||||
} else if !definedCurr && !definedPrev && referencesInput(expr) {
|
||||
return NewError(InputErr, expr.Location, "input document not defined")
|
||||
}
|
||||
|
||||
var err error
|
||||
|
||||
// Check closures contained in this expression.
|
||||
vis := NewGenericVisitor(func(x interface{}) bool {
|
||||
if err != nil {
|
||||
return true
|
||||
}
|
||||
switch x := x.(type) {
|
||||
case *ArrayComprehension:
|
||||
if err = qc.checkInputRec(definedPrev || definedCurr, x.Body); err != nil {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
})
|
||||
|
||||
Walk(vis, expr)
|
||||
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// Check rule bodies referred to by this expression.
|
||||
vis = NewGenericVisitor(func(x interface{}) bool {
|
||||
if err != nil {
|
||||
return true
|
||||
}
|
||||
switch x := x.(type) {
|
||||
case Ref:
|
||||
if x.HasPrefix(DefaultRootRef) {
|
||||
for _, rule := range qc.compiler.GetRules(x.GroundPrefix()) {
|
||||
if err = qc.checkInputRec(definedPrev || definedCurr, rule.Body); err != nil {
|
||||
return true
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
return false
|
||||
})
|
||||
|
||||
Walk(vis, expr)
|
||||
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// referencesInput returns true if expr refers to the input document. This
|
||||
// function will not visit closures.
|
||||
func referencesInput(expr *Expr) bool {
|
||||
|
||||
+2
-6
@@ -1081,12 +1081,8 @@ func TestQueryCompiler(t *testing.T) {
|
||||
{"unsafe vars", "z", "", nil, "", fmt.Errorf("1 error occurred: 1:1: rego_unsafe_var_error: var z is unsafe")},
|
||||
{"safe vars", `data; abc`, `package ex`, []string{"import input.xyz as abc"}, `{}`, `data; input.xyz`},
|
||||
{"reorder", `x != 1; x = 0`, "", nil, "", `x = 0; x != 1`},
|
||||
// {"bad builtin", "deadbeef(1,2,3)", "", nil, "", fmt.Errorf("1 error occurred: 1:1: rego_type_error: undefined built-in function")},
|
||||
{"bad with target", "x = 1 with data.p as null", "", nil, "", fmt.Errorf("1 error occurred: 1:7: rego_type_error: with keyword target must be input")},
|
||||
// wrapping refs in extra terms to cover error handling
|
||||
{"undefined input", `[[true | [data.a.b.d.t, true]], true]`, "", nil, "", fmt.Errorf("5:12: rego_input_error: input document not defined")},
|
||||
{"conflicting input", `[true | data.a.b.d.t with input as 1]`, "", nil, "2", fmt.Errorf("1:9: rego_input_error: input document conflict")},
|
||||
{"conflicting input-2", `sum([1 | data.a.b.d.t with input as 2], x) with input as 3`, "", nil, "", fmt.Errorf("1:10: rego_input_error: input document conflict")},
|
||||
{"check types", "x = data.a.b.c.z; y = null; x = y", "", nil, "", fmt.Errorf("match error\n\tleft : number\n\tright : null")},
|
||||
}
|
||||
|
||||
for _, tc := range tests {
|
||||
@@ -1259,7 +1255,7 @@ func runQueryCompilerTest(t *testing.T, note, q, pkg string, imports []string, i
|
||||
if err == nil {
|
||||
t.Fatalf("Expected error from %v but got: %v", query, result)
|
||||
}
|
||||
if err.Error() != expected.Error() {
|
||||
if !strings.Contains(err.Error(), expected.Error()) {
|
||||
t.Fatalf("Expected error %v but got: %v", expected, err)
|
||||
}
|
||||
}
|
||||
|
||||
+10
-6
@@ -321,11 +321,13 @@ func TestUnset(t *testing.T) {
|
||||
t.Fatalf("Expected unset to succeed for input: %v", err)
|
||||
}
|
||||
|
||||
err = repl.OneShot(ctx, `true = input`)
|
||||
buffer.Reset()
|
||||
repl.OneShot(ctx, `not input`)
|
||||
|
||||
if !strings.Contains(err.Error(), "input document not defined") {
|
||||
t.Fatalf("Expected undefined error but got: %v", err)
|
||||
if buffer.String() != "true\n" {
|
||||
t.Fatalf("Expected unset input to remove input document: %v", buffer.String())
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
func TestOneShotEmptyBufferOneExpr(t *testing.T) {
|
||||
@@ -827,12 +829,14 @@ func TestEvalBodyWith(t *testing.T) {
|
||||
repl := newRepl(store, &buffer)
|
||||
|
||||
repl.OneShot(ctx, `p = true { input.foo = "bar" }`)
|
||||
err := repl.OneShot(ctx, "p")
|
||||
repl.OneShot(ctx, "p")
|
||||
|
||||
if err == nil || !strings.Contains(err.Error(), "input document not defined") {
|
||||
t.Fatalf("Expected input document undefined error")
|
||||
if buffer.String() != "undefined\n" {
|
||||
t.Fatalf("Expected undefined but got: %v", buffer.String())
|
||||
}
|
||||
|
||||
buffer.Reset()
|
||||
|
||||
repl.OneShot(ctx, `p with input.foo as "bar"`)
|
||||
|
||||
result := buffer.String()
|
||||
|
||||
@@ -54,11 +54,6 @@ undef = true { false }`
|
||||
p = [1, 2, 3, 4] { true }
|
||||
q = {"a": 1, "b": 2} { true }`
|
||||
|
||||
testMod3 := `package testmod
|
||||
|
||||
p = true { loopback with input as true }
|
||||
loopback = input { true }`
|
||||
|
||||
testMod4 := `package testmod
|
||||
|
||||
p = true { true }
|
||||
@@ -168,24 +163,6 @@ p = true { false }`
|
||||
tr{"PUT", "/policies/test", testMod1, 200, ""},
|
||||
tr{"GET", "/data/testmod/g?input=req1%3A%7B%22a%22%3A%5B1%5D%7D&input=req2%3A%7B%22b%22%3A%5B0%2C1%5D%7D", "", 200, `{"result": true}`},
|
||||
}},
|
||||
{"get missing input", []tr{
|
||||
tr{"PUT", "/policies/test", testMod1, 200, ""},
|
||||
tr{"GET", "/data/testmod/g", "", 400, `{
|
||||
"code": "invalid_parameter",
|
||||
"errors": [
|
||||
{
|
||||
"code": "rego_input_error",
|
||||
"location": {
|
||||
"col": 12,
|
||||
"file": "test",
|
||||
"row": 10
|
||||
},
|
||||
"message": "input document not defined"
|
||||
}
|
||||
],
|
||||
"message": "input document is missing or conflicts with query"
|
||||
}`},
|
||||
}},
|
||||
{"get with input (missing input value)", []tr{
|
||||
tr{"PUT", "/policies/test", testMod1, 200, ""},
|
||||
tr{"GET", "/data/testmod/g?input=req1%3A%7B%22a%22%3A%5B1%5D%7D", "", 200, "{}"}, // req2 not specified
|
||||
@@ -257,24 +234,6 @@ p = true { false }`
|
||||
tr{"PUT", "/policies/test", testMod1, 200, ""},
|
||||
tr{"POST", "/data/testmod/gt1", `{"input": {"req1": 2}}`, 200, `{"result": true}`},
|
||||
}},
|
||||
{"post missing input", []tr{
|
||||
tr{"PUT", "/policies/test", testMod1, 200, ""},
|
||||
tr{"POST", "/data/testmod/gt1", ``, 400, `{
|
||||
"code": "invalid_parameter",
|
||||
"message": "input document is missing or conflicts with query",
|
||||
"errors": [
|
||||
{
|
||||
"code": "rego_input_error",
|
||||
"location": {
|
||||
"file": "test",
|
||||
"row": 12,
|
||||
"col": 14
|
||||
},
|
||||
"message": "input document not defined"
|
||||
}
|
||||
]
|
||||
}`},
|
||||
}},
|
||||
{"post malformed input", []tr{
|
||||
tr{"POST", "/data/deadbeef", `{"input": @}`, 400, `{
|
||||
"code": "invalid_parameter",
|
||||
@@ -299,24 +258,6 @@ p = true { false }`
|
||||
"message": "error(s) occurred while evaluating query"
|
||||
}`},
|
||||
}},
|
||||
{"input conflict", []tr{
|
||||
tr{"PUT", "/policies/test", testMod3, 200, ""},
|
||||
tr{"POST", "/data/testmod/p", `{"input": false}`, 400, `{
|
||||
"code": "invalid_parameter",
|
||||
"errors": [
|
||||
{
|
||||
"code": "rego_input_error",
|
||||
"location": {
|
||||
"col": 12,
|
||||
"file": "test",
|
||||
"row": 3
|
||||
},
|
||||
"message": "input document conflict"
|
||||
}
|
||||
],
|
||||
"message": "input document is missing or conflicts with query"
|
||||
}`},
|
||||
}},
|
||||
{"query wildcards omitted", []tr{
|
||||
tr{"PATCH", "/data/x", `[{"op": "add", "path": "/", "value": [1,2,3,4]}]`, 204, ""},
|
||||
tr{"GET", "/query?q=data.x[_]%20=%20x", "", 200, `{"result": [{"x": 1}, {"x": 2}, {"x": 3}, {"x": 4}]}`},
|
||||
|
||||
@@ -1186,7 +1186,7 @@ loopback = input { true }`})
|
||||
|
||||
assertTopDown(t, compiler, store, "loopback", []string{"z", "loopback"}, `{"foo": 1}`, `{"foo": 1}`)
|
||||
|
||||
assertTopDown(t, compiler, store, "loopback undefined", []string{"z", "loopback"}, ``, fmt.Errorf("input document not defined"))
|
||||
assertTopDown(t, compiler, store, "loopback undefined", []string{"z", "loopback"}, ``, ``)
|
||||
|
||||
assertTopDown(t, compiler, store, "simple", []string{"z", "p"}, `{
|
||||
"req1": {"foo": 4},
|
||||
|
||||
Reference in New Issue
Block a user