fix(#210): allowed value log in response policy evaluation (#212)

* fix(#210): allowed value simplification

* refactor: use allowed-inspired func

* fix: lax allowed evaluator for logging purposes

* refactor: removed useless log

* refactor: typo and better comment

* refactor: use empty

Co-authored-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>

* refactor: removed lax allow and reworked results processing to fit allowed eval

---------

Co-authored-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>

core/opaevaluator.go

@@ -344,40 +344,26 @@ func (evaluator *OPAEvaluator) Evaluate(logger *logrus.Entry) (interface{}, erro
 		"policy_name": evaluator.PolicyName,
 	}).Observe(float64(opaEvaluationTime.Milliseconds()))
 
+	allowed, responseBodyOverwriter := processResults(results)
 	logger.WithFields(logrus.Fields{
 		"evaluationTimeMicroseconds": opaEvaluationTime.Microseconds(),
 		"policyName":                 evaluator.PolicyName,
 		"partialEval":                false,
-		"allowed":                    results.Allowed(),
+		"allowed":                    allowed,
 		"resultsLength":              len(results),
 		"matchedPath":                evaluator.routerInfo.MatchedPath,
 		"requestedPath":              evaluator.routerInfo.RequestedPath,
 		"method":                     evaluator.routerInfo.Method,
 	}).Debug("policy evaluation completed")
 
-	if results.Allowed() {
-		logger.WithFields(logrus.Fields{
-			"policyName":    evaluator.PolicyName,
-			"allowed":       results.Allowed(),
-			"resultsLength": len(results),
-		}).Tracef("policy results")
-		return nil, nil
-	}
-	// The results returned by OPA are a list of Results object with fields:
-	// - Expressions: list of list
-	// - Bindings: object
-	// e.g. [{Expressions:[[map["element": true]]] Bindings:map[]}]
-	// Since we are ALWAYS querying ONE specific policy the result length could not be greater than 1
-	if len(results) == 1 {
-		if exprs := results[0].Expressions; len(exprs) == 1 {
-			if value, ok := exprs[0].Value.([]interface{}); ok && value != nil && len(value) != 0 {
-				return value[0], nil
-			}
-		}
-	}
 	logger.WithFields(logrus.Fields{
 		"policyName": evaluator.PolicyName,
-	}).Error("policy resulted in not allowed")
+		"allowed":    allowed,
+	}).Info("policy result")
+
+	if allowed {
+		return responseBodyOverwriter, nil
+	}
 	return nil, fmt.Errorf("RBAC policy evaluation failed, user is not allowed")
 }
 
@@ -444,3 +430,29 @@ func LoadRegoModule(rootDirectory string) (*OPAModuleConfig, error) {
 		Content: string(fileContent),
 	}, nil
 }
+
+func processResults(results rego.ResultSet) (allowed bool, responseBodyOverwriter any) {
+	// Use strict allowed check for basic request flow allow policies.
+	if results.Allowed() {
+		allowed = true
+		return
+	}
+
+	// Here extract first result set to get the response body for the response policy evaluation.
+	// The results returned by OPA are a list of Results object with fields:
+	// - Expressions: list of list
+	// - Bindings: object
+	// e.g. [{Expressions:[[map["element": true]]] Bindings:map[]}]
+	// Since we are ALWAYS querying ONE specific policy the result length could not be greater than 1
+	if len(results) == 1 {
+		if exprs := results[0].Expressions; len(exprs) == 1 {
+			if value, ok := exprs[0].Value.([]interface{}); ok && value != nil && len(value) != 0 {
+				allowed = true
+				responseBodyOverwriter = value[0]
+				return
+			}
+		}
+	}
+
+	return
+}

core/sdk_test.go

@@ -493,6 +493,7 @@ func TestEvaluateResponsePolicy(t *testing.T) {
 		expectedBody       string
 		expectedErr        bool
 		expectedErrMessage string
+		notAllowed         bool
 	}
 
 	t.Run("evaluate response", func(t *testing.T) {
@@ -527,6 +528,20 @@ func TestEvaluateResponsePolicy(t *testing.T) {
 
 				expectedBody: `{"f1":"censored","foo":"bar"}`,
 			},
+			"with policy failure": {
+				method: http.MethodGet,
+				path:   "/users/",
+				opaModuleContent: `
+				package policies
+				responsepolicy [body] {
+					false
+					body := input.response.body
+				}`,
+				expectedErr:        true,
+				expectedErrMessage: "RBAC policy evaluation failed, user is not allowed",
+				expectedBody:       "",
+				notAllowed:         true,
+			},
 			"with mongo query and body changed": {
 				method: http.MethodGet,
 				path:   "/users/",
@@ -595,7 +610,12 @@ func TestEvaluateResponsePolicy(t *testing.T) {
 				} else {
 					require.NoError(t, err)
 				}
-				require.JSONEq(t, testCase.expectedBody, string(actual))
+
+				if testCase.expectedBody == "" {
+					require.Empty(t, string(actual))
+				} else {
+					require.JSONEq(t, testCase.expectedBody, string(actual))
+				}
 
 				t.Run("logger", func(t *testing.T) {
 					var actual *logrus.Entry
@@ -609,7 +629,7 @@ func TestEvaluateResponsePolicy(t *testing.T) {
 					require.NotNil(t, actual)
 					delete(actual.Data, "evaluationTimeMicroseconds")
 					require.Equal(t, logrus.Fields{
-						"allowed":       false,
+						"allowed":       !testCase.notAllowed,
 						"requestedPath": testCase.path,
 						"matchedPath":   evaluatorInfo.evaluatorOptions.RouterInfo.MatchedPath,
 						"method":        testCase.method,

go.mod

@@ -12,6 +12,7 @@ require (
 	github.com/mia-platform/glogger/v2 v2.1.3
 	github.com/open-policy-agent/opa v0.53.1
 	github.com/prometheus/client_golang v1.16.0
+	github.com/prometheus/common v0.42.0
 	github.com/samber/lo v1.38.1
 	github.com/sirupsen/logrus v1.9.3
 	github.com/stretchr/testify v1.8.4
@@ -57,7 +58,6 @@ require (
 	github.com/perimeterx/marshmallow v1.1.4 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/prometheus/client_model v0.3.0 // indirect
-	github.com/prometheus/common v0.42.0 // indirect
 	github.com/prometheus/procfs v0.10.1 // indirect
 	github.com/rcrowley/go-metrics v0.0.0-20201227073835-cf1acfcdf475 // indirect
 	github.com/spf13/afero v1.9.2 // indirect