Automate query parameters parsing and validation. (#1755)

Use https://www.gorillatoolkit.org/pkg/schema and https://github.com/asaskevich/govalidator to automate parsing and validation of query parameters.

go.list

@@ -26,6 +26,7 @@ github.com/golang/protobuf v1.3.1
 github.com/gomodule/redigo v2.0.0+incompatible
 github.com/google/go-querystring v0.0.0-20160401233042-9235644dd9e5
 github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1
+github.com/gorilla/schema v1.1.0
 github.com/graph-gophers/graphql-go v0.0.0-20190225005345-3e8838d4614c
 github.com/guregu/null v2.1.3-0.20151024101046-79c5bd36b615+incompatible
 github.com/hashicorp/golang-lru v0.5.0

go.mod

@@ -25,6 +25,7 @@ require (
 	github.com/goji/httpauth v0.0.0-20160601135302-2da839ab0f4d
 	github.com/gomodule/redigo v2.0.0+incompatible
 	github.com/google/go-querystring v0.0.0-20160401233042-9235644dd9e5 // indirect
+	github.com/gorilla/schema v1.1.0
 	github.com/graph-gophers/graphql-go v0.0.0-20190225005345-3e8838d4614c
 	github.com/guregu/null v2.1.3-0.20151024101046-79c5bd36b615+incompatible
 	github.com/hashicorp/golang-lru v0.5.0 // indirect

go.sum

@@ -53,6 +53,8 @@ github.com/google/go-querystring v0.0.0-20160401233042-9235644dd9e5 h1:oERTZ1buO
 github.com/google/go-querystring v0.0.0-20160401233042-9235644dd9e5/go.mod h1:odCYkC5MyYFN7vkCjXpyrEuKhc/BUO6wN/zVPAxq5ck=
 github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1 h1:EGx4pi6eqNxGaHF6qqu48+N2wcFQ5qg5FXgOdqsJ5d8=
 github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1/go.mod h1:wJfORRmW1u3UXTncJ5qlYoELFm8eSnnEO6hX4iZ3EWY=
+github.com/gorilla/schema v1.1.0 h1:CamqUDOFUBqzrvxuz2vEwo8+SUdwsluFh7IlzJh30LY=
+github.com/gorilla/schema v1.1.0/go.mod h1:kgLaKoK1FELgZqMAVxx/5cbj0kT+57qxUrAlIO2eleU=
 github.com/graph-gophers/graphql-go v0.0.0-20190225005345-3e8838d4614c h1:YyFUsspLqAt3noyPCLz7EFK/o1LpC1j/6MjU0bSVOQ4=
 github.com/graph-gophers/graphql-go v0.0.0-20190225005345-3e8838d4614c/go.mod h1:uJhtPXrcJLqyi0H5IuMFh+fgW+8cMMakK3Txrbk/WJE=
 github.com/guregu/null v2.1.3-0.20151024101046-79c5bd36b615+incompatible h1:SZmF1M6CdAm4MmTPYYTG+x9EC8D3FOxUq9S4D37irQg=

services/horizon/internal/actions/helpers.go

@@ -6,12 +6,15 @@ import (
 	"mime"
 	"net/http"
 	"net/url"
+	"reflect"
 	"regexp"
 	"strconv"
 	"strings"
 	"unicode/utf8"
 
+	"github.com/asaskevich/govalidator"
 	"github.com/go-chi/chi"
+	"github.com/gorilla/schema"
 
 	"github.com/stellar/go/amount"
 	"github.com/stellar/go/services/horizon/internal/assets"
@@ -711,3 +714,125 @@ func FullURL(ctx context.Context) *url.URL {
 	}
 	return url
 }
+
+// Note from chi: it is a good idea to set a Decoder instance as a package
+// global, because it caches meta-data about structs, and an instance can be
+// shared safely:
+var decoder = schema.NewDecoder()
+
+// GetParams fills a struct with values read from a request's query parameters.
+func GetParams(dst interface{}, r *http.Request) error {
+	query := r.URL.Query()
+
+	// Merge chi's URLParams with URL Query Params. Given
+	// `/accounts/{account_id}/transactions?foo=bar`, chi's URLParams will
+	// contain `account_id` and URL Query params will contain `foo`.
+	if rctx := chi.RouteContext(r.Context()); rctx != nil {
+		for _, key := range rctx.URLParams.Keys {
+			val := query.Get(key)
+			if len(val) > 0 {
+				return problem.MakeInvalidFieldProblem(
+					key,
+					errors.New("The parameter should not be included in the request"),
+				)
+			}
+
+			query.Set(key, rctx.URLParam(key))
+		}
+	}
+
+	decoder.IgnoreUnknownKeys(true)
+	if err := decoder.Decode(dst, query); err != nil {
+		return errors.Wrap(err, "error decoding Request query")
+	}
+
+	if _, err := govalidator.ValidateStruct(dst); err != nil {
+		field, message := getErrorFieldMessage(err)
+		err = problem.MakeInvalidFieldProblem(
+			getSchemaTag(dst, field),
+			errors.New(message),
+		)
+
+		return err
+	}
+
+	if v, ok := dst.(Validateable); ok {
+		if err := v.Validate(); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+func getSchemaTag(params interface{}, field string) string {
+	v := reflect.ValueOf(params).Elem()
+	qt := v.Type()
+	f, _ := qt.FieldByName(field)
+	return f.Tag.Get("schema")
+}
+
+func validateAssetParams(aType, code, issuer, prefix string) error {
+	// If asset type is not present but code or issuer are, then there is a
+	// missing parameter and the request is unprocessable.
+	if len(aType) == 0 {
+		if len(code) > 0 || len(issuer) > 0 {
+			return problem.MakeInvalidFieldProblem(
+				prefix+"_asset_type",
+				errors.New("Missing parameter"),
+			)
+		}
+
+		return nil
+	}
+
+	t, err := assets.Parse(aType)
+	if err != nil {
+		return problem.MakeInvalidFieldProblem(
+			prefix+"_asset_type",
+			err,
+		)
+	}
+
+	var validLen int
+	switch t {
+	case xdr.AssetTypeAssetTypeNative:
+		// If asset type is native, issuer or code should not be included in the
+		// request
+		switch {
+		case len(code) > 0:
+			return problem.MakeInvalidFieldProblem(
+				prefix+"_asset_code",
+				errors.New("native asset does not have a code"),
+			)
+		case len(issuer) > 0:
+			return problem.MakeInvalidFieldProblem(
+				prefix+"_asset_issuer",
+				errors.New("native asset does not have an issuer"),
+			)
+		}
+
+		return nil
+	case xdr.AssetTypeAssetTypeCreditAlphanum4:
+		validLen = len(xdr.AssetAlphaNum4{}.AssetCode)
+	case xdr.AssetTypeAssetTypeCreditAlphanum12:
+		validLen = len(xdr.AssetAlphaNum12{}.AssetCode)
+	}
+
+	codeLen := len(code)
+	if codeLen == 0 || codeLen > validLen {
+		return problem.MakeInvalidFieldProblem(
+			prefix+"_asset_code",
+			errors.New("Asset code must be 1-12 alphanumeric characters"),
+		)
+	}
+
+	if len(issuer) == 0 {
+		return problem.MakeInvalidFieldProblem(
+			prefix+"_asset_issuer",
+			errors.New("Missing parameter"),
+		)
+	}
+
+	return nil
+}

services/horizon/internal/actions/helpers_test.go

@@ -16,6 +16,7 @@ import (
 	"github.com/stellar/go/services/horizon/internal/test"
 	"github.com/stellar/go/services/horizon/internal/toid"
 	"github.com/stellar/go/support/db"
+	"github.com/stellar/go/support/errors"
 	"github.com/stellar/go/support/render/problem"
 	"github.com/stellar/go/xdr"
 )
@@ -570,6 +571,108 @@ func TestFullURL(t *testing.T) {
 	tt.Assert.Equal("http:///foo-bar/blah?limit=2&cursor=123456", url.String())
 }
 
+func TestGetParams(t *testing.T) {
+	tt := test.Start(t)
+	defer tt.Finish()
+
+	type QueryParams struct {
+		SellingBuyingAssetQueryParams `valid:"-"`
+		Account                       string `schema:"account_id" valid:"accountID"`
+	}
+
+	account := "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H"
+	usd := xdr.MustNewCreditAsset("USD", account)
+
+	// Simulate chi's URL params. The following would be equivalent to having a
+	// chi route like the following `/accounts/{account_id}`
+	urlParams := map[string]string{
+		"account_id":           account,
+		"selling_asset_type":   "credit_alphanum4",
+		"selling_asset_code":   "USD",
+		"selling_asset_issuer": account,
+	}
+
+	r := makeAction("/transactions?limit=2&cursor=123456&order=desc", urlParams).R
+	qp := QueryParams{}
+	err := GetParams(&qp, r)
+
+	tt.Assert.NoError(err)
+	tt.Assert.Equal(account, qp.Account)
+	tt.Assert.True(usd.Equals(*qp.Selling()))
+
+	urlParams = map[string]string{
+		"account_id":         account,
+		"selling_asset_type": "native",
+	}
+
+	r = makeAction("/transactions?limit=2&cursor=123456&order=desc", urlParams).R
+	qp = QueryParams{}
+	err = GetParams(&qp, r)
+
+	tt.Assert.NoError(err)
+	native := xdr.MustNewNativeAsset()
+	tt.Assert.True(native.Equals(*qp.Selling()))
+
+	urlParams = map[string]string{"account_id": "1"}
+	r = makeAction("/transactions?limit=2&cursor=123456&order=desc", urlParams).R
+	qp = QueryParams{}
+	err = GetParams(&qp, r)
+
+	if tt.Assert.IsType(&problem.P{}, err) {
+		p := err.(*problem.P)
+		tt.Assert.Equal("bad_request", p.Type)
+		tt.Assert.Equal("account_id", p.Extras["invalid_field"])
+		tt.Assert.Equal(
+			"Account ID must start with `G` and contain 56 alphanum characters",
+			p.Extras["reason"],
+		)
+	}
+
+	urlParams = map[string]string{
+		"account_id": account,
+	}
+	r = makeAction(fmt.Sprintf("/transactions?account_id=%s", account), urlParams).R
+	err = GetParams(&qp, r)
+
+	tt.Assert.Error(err)
+	if tt.Assert.IsType(&problem.P{}, err) {
+		p := err.(*problem.P)
+		tt.Assert.Equal("bad_request", p.Type)
+		tt.Assert.Equal("account_id", p.Extras["invalid_field"])
+		tt.Assert.Equal(
+			"The parameter should not be included in the request",
+			p.Extras["reason"],
+		)
+	}
+}
+
+type ParamsValidator struct {
+	Account string `schema:"account_id" valid:"required"`
+}
+
+func (pv ParamsValidator) Validate() error {
+	return problem.MakeInvalidFieldProblem(
+		"Name",
+		errors.New("Invalid"),
+	)
+}
+
+func TestGetParamsCustomValidator(t *testing.T) {
+	tt := test.Start(t)
+	defer tt.Finish()
+
+	urlParams := map[string]string{"account_id": "1"}
+	r := makeAction("/transactions", urlParams).R
+	qp := ParamsValidator{}
+	err := GetParams(&qp, r)
+
+	if tt.Assert.IsType(&problem.P{}, err) {
+		p := err.(*problem.P)
+		tt.Assert.Equal("bad_request", p.Type)
+		tt.Assert.Equal("Name", p.Extras["invalid_field"])
+	}
+}
+
 func makeTestAction() *Base {
 	return makeAction("/foo-bar/blah?limit=2&cursor=123456", testURLParams())
 }

services/horizon/internal/actions/offer.go

@@ -9,7 +9,6 @@ import (
 	"github.com/stellar/go/services/horizon/internal/resourceadapter"
 	"github.com/stellar/go/support/errors"
 	"github.com/stellar/go/support/render/hal"
-	"github.com/stellar/go/xdr"
 )
 
 // GetOfferByID is the action handler for the /offers/{id} endpoint
@@ -53,6 +52,17 @@ func (handler GetOfferByID) GetResource(
 	return offerResponse, nil
 }
 
+// OffersQuery query struct for offers end-point
+type OffersQuery struct {
+	SellingBuyingAssetQueryParams `valid:"-"`
+	Seller                        string `schema:"seller" valid:"accountID,optional"`
+}
+
+// Validate runs custom validations.
+func (q OffersQuery) Validate() error {
+	return q.SellingBuyingAssetQueryParams.Validate()
+}
+
 // GetOffersHandler is the action handler for the /offers endpoint
 type GetOffersHandler struct {
 }
@@ -63,31 +73,22 @@ func (handler GetOffersHandler) GetResourcePage(
 	r *http.Request,
 ) ([]hal.Pageable, error) {
 	ctx := r.Context()
-	pq, err := GetPageQuery(r)
+	qp := OffersQuery{}
+	err := GetParams(&qp, r)
 	if err != nil {
 		return nil, err
 	}
 
-	seller, err := GetString(r, "seller")
+	pq, err := GetPageQuery(r)
 	if err != nil {
 		return nil, err
 	}
 
-	var selling *xdr.Asset
-	if sellingAsset, found := MaybeGetAsset(r, "selling_"); found {
-		selling = &sellingAsset
-	}
-
-	var buying *xdr.Asset
-	if buyingAsset, found := MaybeGetAsset(r, "buying_"); found {
-		buying = &buyingAsset
-	}
-
 	query := history.OffersQuery{
 		PageQuery: pq,
-		SellerID:  seller,
-		Selling:   selling,
-		Buying:    buying,
+		SellerID:  qp.Seller,
+		Selling:   qp.Selling(),
+		Buying:    qp.Buying(),
 	}
 
 	historyQ, err := historyQFromRequest(r)

services/horizon/internal/actions/offer_test.go

@@ -241,6 +241,27 @@ func TestGetOffersHandler(t *testing.T) {
 		for _, offer := range offers {
 			tt.Assert.Equal(issuer.Address(), offer.Seller)
 		}
+
+		_, err = handler.GetResourcePage(
+			httptest.NewRecorder(),
+			makeRequest(
+				t,
+				map[string]string{
+					"seller": "GCXEWJ6U4KPGTNTBY5HX4WQ2EEVPWV2QKXEYIQ32IDYIX",
+				},
+				map[string]string{},
+				q.Session,
+			),
+		)
+		tt.Assert.Error(err)
+		tt.Assert.IsType(&problem.P{}, err)
+		p := err.(*problem.P)
+		tt.Assert.Equal("bad_request", p.Type)
+		tt.Assert.Equal("seller", p.Extras["invalid_field"])
+		tt.Assert.Equal(
+			"Account ID must start with `G` and contain 56 alphanum characters",
+			p.Extras["reason"],
+		)
 	})
 
 	t.Run("Filter by selling asset", func(t *testing.T) {