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:
Torin Sandall
2017-05-09 09:36:09 -07:00
parent a9567ff6a8
commit b23cb4e30f
5 changed files with 13 additions and 140 deletions
-68
View File
@@ -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
View File
@@ -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
View File
@@ -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()
-59
View File
@@ -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}]}`},
+1 -1
View File
@@ -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},