feat: return query_parameters_to_remove with OkResponse (#621)

Envoy External Auth v3 API supports the `query_parameters_to_remove` attribute with
`OkResponse`.

See https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/auth/v3/external_auth.proto#service-auth-v3-okhttpresponse

The change allows to return from OPA which query parameters should be removed
from the upstream query.

Signed-off-by: Anthony Regeda <regedaster@gmail.com>

envoyauth/response.go

@@ -114,43 +114,51 @@ func (result *EvalResult) IsAllowed() (bool, error) {
 	return false, result.invalidDecisionErr()
 }
 
-// GetRequestHTTPHeadersToRemove - returns the http headers to remove from the original request before dispatching
-// it to the upstream
-func (result *EvalResult) GetRequestHTTPHeadersToRemove() ([]string, error) {
-	headersToRemove := []string{}
-
+func (result *EvalResult) getStringSliceFromDecision(fieldName string) ([]string, error) {
 	switch decision := result.Decision.(type) {
 	case bool:
-		return headersToRemove, nil
+		return nil, nil
 	case map[string]interface{}:
 		var ok bool
 		var val interface{}
 
-		if val, ok = decision["request_headers_to_remove"]; !ok {
-			return headersToRemove, nil
+		if val, ok = decision[fieldName]; !ok {
+			return nil, nil
 		}
 
 		switch val := val.(type) {
 		case []string:
 			return val, nil
 		case []interface{}:
-			for _, vval := range val {
-				header, ok := vval.(string)
+			ss := make([]string, len(val))
+			for i, v := range val {
+				s, ok := v.(string)
 				if !ok {
-					return nil, fmt.Errorf("type assertion error, expected request_headers_to_remove value to be of type 'string' but got '%T'", vval)
+					return nil, fmt.Errorf("type assertion error, expected %s value to be of type 'string' but got '%T'", fieldName, v)
 				}
-
-				headersToRemove = append(headersToRemove, header)
+				ss[i] = s
 			}
-			return headersToRemove, nil
+			return ss, nil
 		default:
-			return nil, fmt.Errorf("type assertion error, expected request_headers_to_remove to be of type '[]string' but got '%T'", val)
+			return nil, fmt.Errorf("type assertion error, expected %s to be of type '[]string' but got '%T'", fieldName, val)
 		}
 	}
 
 	return nil, result.invalidDecisionErr()
 }
 
+// GetRequestQueryParametersToRemove - returns the query parameters to remove from the original request before dispatching
+// it to the upstream
+func (result *EvalResult) GetRequestQueryParametersToRemove() ([]string, error) {
+	return result.getStringSliceFromDecision("query_parameters_to_remove")
+}
+
+// GetRequestHTTPHeadersToRemove - returns the http headers to remove from the original request before dispatching
+// it to the upstream
+func (result *EvalResult) GetRequestHTTPHeadersToRemove() ([]string, error) {
+	return result.getStringSliceFromDecision("request_headers_to_remove")
+}
+
 // GetResponseHTTPHeaders - returns the http headers to return if they are part of the decision
 func (result *EvalResult) GetResponseHTTPHeaders() (http.Header, error) {
 	var responseHeaders = make(http.Header)

envoyauth/response_test.go

@@ -46,6 +46,74 @@ func TestIsAllowed(t *testing.T) {
 	}
 }
 
+func TestGetRequestQueryParametersToRemove(t *testing.T) {
+	tests := map[string]struct {
+		decision interface{}
+		exp      []string
+		wantErr  bool
+	}{
+		"bool_eval_result": {
+			true,
+			nil,
+			false,
+		},
+		"invalid_eval_result": {
+			"hello",
+			nil,
+			true,
+		},
+		"empty_map_result": {
+			map[string]interface{}{},
+			nil,
+			false,
+		},
+		"bad_param_value": {
+			map[string]interface{}{"query_parameters_to_remove": "test"},
+			nil,
+			true,
+		},
+		"string_array_param_value": {
+			map[string]interface{}{"query_parameters_to_remove": []string{"foo", "bar"}},
+			[]string{"foo", "bar"},
+			false,
+		},
+		"interface_array_param_value": {
+			map[string]interface{}{"query_parameters_to_remove": []interface{}{"foo", "bar", "fuz"}},
+			[]string{"foo", "bar", "fuz"},
+			false,
+		},
+		"interface_array_bad_param_value": {
+			map[string]interface{}{"query_parameters_to_remove": []interface{}{1}},
+			nil,
+			true,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			er := EvalResult{
+				Decision: tc.decision,
+			}
+
+			result, err := er.GetRequestQueryParametersToRemove()
+
+			if tc.wantErr {
+				if err == nil {
+					t.Fatal("Expected error but got nil")
+				}
+			} else {
+				if err != nil {
+					t.Fatalf("Unexpected error %v", err)
+				}
+
+				if !reflect.DeepEqual(tc.exp, result) {
+					t.Fatalf("Expected result %v but got %v", tc.exp, result)
+				}
+			}
+		})
+	}
+}
+
 func TestGetRequestHTTPHeadersToRemove(t *testing.T) {
 	tests := map[string]struct {
 		decision interface{}
@@ -54,22 +122,22 @@ func TestGetRequestHTTPHeadersToRemove(t *testing.T) {
 	}{
 		"bool_eval_result": {
 			true,
-			[]string{},
+			nil,
 			false,
 		},
 		"invalid_eval_result": {
 			"hello",
-			[]string{},
+			nil,
 			true,
 		},
 		"empty_map_result": {
 			map[string]interface{}{},
-			[]string{},
+			nil,
 			false,
 		},
 		"bad_header_value": {
 			map[string]interface{}{"request_headers_to_remove": "test"},
-			[]string{},
+			nil,
 			true,
 		},
 		"string_array_header_value": {
@@ -84,7 +152,7 @@ func TestGetRequestHTTPHeadersToRemove(t *testing.T) {
 		},
 		"interface_array_bad_header_value": {
 			map[string]interface{}{"request_headers_to_remove": []interface{}{1}},
-			[]string{},
+			nil,
 			true,
 		},
 	}

internal/internal.go

@@ -525,11 +525,20 @@ func (p *envoyExtAuthzGrpcServer) check(ctx context.Context, req interface{}) (*
 				return nil, stop, internalErr
 			}
 
+			var queryParamsToRemove []string
+			queryParamsToRemove, err = result.GetRequestQueryParametersToRemove()
+			if err != nil {
+				err = errors.Wrap(err, "failed to get request query parameters to remove")
+				internalErr = newInternalError(EnvoyAuthResultErr, err)
+				return nil, stop, internalErr
+			}
+
 			resp.HttpResponse = &ext_authz_v3.CheckResponse_OkResponse{
 				OkResponse: &ext_authz_v3.OkHttpResponse{
-					Headers:              responseHeaders,
-					HeadersToRemove:      headersToRemove,
-					ResponseHeadersToAdd: responseHeadersToAdd,
+					Headers:                 responseHeaders,
+					HeadersToRemove:         headersToRemove,
+					ResponseHeadersToAdd:    responseHeadersToAdd,
+					QueryParametersToRemove: queryParamsToRemove,
 				},
 			}
 		} else {

internal/internal_test.go

@@ -8,8 +8,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	ext_type_v2 "github.com/envoyproxy/go-control-plane/envoy/type"
-	ext_type_v3 "github.com/envoyproxy/go-control-plane/envoy/type/v3"
 	"net/http"
 	"net/http/httptest"
 	"reflect"
@@ -21,6 +19,8 @@ import (
 	ext_core "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
 	ext_authz_v2 "github.com/envoyproxy/go-control-plane/envoy/service/auth/v2"
 	ext_authz "github.com/envoyproxy/go-control-plane/envoy/service/auth/v3"
+	ext_type_v2 "github.com/envoyproxy/go-control-plane/envoy/type"
+	ext_type_v3 "github.com/envoyproxy/go-control-plane/envoy/type/v3"
 	_structpb "github.com/golang/protobuf/ptypes/struct"
 	"github.com/prometheus/client_golang/prometheus"
 	"google.golang.org/genproto/googleapis/rpc/code"
@@ -1518,6 +1518,50 @@ func TestCheckAllowBooleanDecisionDynamicMetadataDecisionID(t *testing.T) {
 	assertDynamicMetadataDecisionID(t, output.GetDynamicMetadata())
 }
 
+func TestCheckAllowObjectDecisionReqQueryParamsToRemove(t *testing.T) {
+	var req ext_authz.CheckRequest
+	if err := util.Unmarshal([]byte(exampleAllowedRequest), &req); err != nil {
+		panic(err)
+	}
+
+	module := `
+		package envoy.authz
+
+		default allow = true
+
+		query_parameters_to_remove := ["foo", "bar"]
+
+		result["allowed"] = allow
+		result["query_parameters_to_remove"] = query_parameters_to_remove`
+
+	server := testAuthzServerWithModule(module, "envoy/authz/result", nil, withCustomLogger(&testPlugin{}))
+	ctx := context.Background()
+	output, err := server.Check(ctx, &req)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if output.Status.Code != int32(code.Code_OK) {
+		t.Fatalf("Expected request to be allowed but got: %v", output)
+	}
+
+	response := output.GetOkResponse()
+	if response == nil {
+		t.Fatal("Expected OkHttpResponse struct but got nil")
+	}
+
+	queryParams := response.GetQueryParametersToRemove()
+	if len(queryParams) != 2 {
+		t.Fatalf("Expected two query params but got %v", len(queryParams))
+	}
+
+	expectedQueryParams := []string{"foo", "bar"}
+
+	if !reflect.DeepEqual(expectedQueryParams, queryParams) {
+		t.Fatalf("Expected query params %v but got %v", expectedQueryParams, queryParams)
+	}
+}
+
 func TestCheckAllowObjectDecisionReqHeadersToRemove(t *testing.T) {
 	var req ext_authz.CheckRequest
 	if err := util.Unmarshal([]byte(exampleAllowedRequestParsedPath), &req); err != nil {