Make service_plan payload validation more strict

It was possible to pass fileds like `fileds[service_broker.service_offering]` which make no sense as offerings have relationships to brokers, but not vice versa. Also add `spaces.organization` to the valid set of values of the `include` param. This is needed by the `cf marketplace` command. Co-authored-by: Yusmen Zabanov <[email protected]>
cloudfoundry · Aug 15, 2024 · 456ff3d · 456ff3d
1 parent abc683f
commit 456ff3d
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 19 deletions.
diff --git a/api/payloads/params/include.go b/api/payloads/params/include.go
@@ -4,23 +4,13 @@ import (
 	"fmt"
 	"net/url"
 	"strings"
-
-	"code.cloudfoundry.org/korifi/api/payloads/validation"
-	jellidation "github.com/jellydator/validation"
 )
 
 type IncludeResourceRule struct {
 	RelationshipPath []string
 	Fields           []string
 }
 
-func (r IncludeResourceRule) Validate() error {
-	return jellidation.ValidateStruct(&r,
-		jellidation.Field(&r.RelationshipPath, jellidation.Each(validation.OneOf("service_offering", "service_broker"))),
-		jellidation.Field(&r.Fields, jellidation.Each(validation.OneOf("guid", "name"))),
-	)
-}
-
 func ParseFields(values url.Values) []IncludeResourceRule {
 	includes := []IncludeResourceRule{}
 	fmt.Printf("values = %+v\n", values)

diff --git a/api/payloads/service_plan.go b/api/payloads/service_plan.go
@@ -5,6 +5,7 @@ import (
 	"net/url"
 	"regexp"
 	"strconv"
+	"strings"
 
 	"code.cloudfoundry.org/korifi/api/payloads/params"
 	"code.cloudfoundry.org/korifi/api/payloads/parse"
@@ -27,10 +28,34 @@ type ServicePlanList struct {
 
 func (l ServicePlanList) Validate() error {
 	return jellidation.ValidateStruct(&l,
-		jellidation.Field(&l.IncludeResourceRules),
+		jellidation.Field(&l.IncludeResourceRules, jellidation.Each(jellidation.By(func(value any) error {
+			rule, ok := value.(params.IncludeResourceRule)
+			if !ok {
+				return fmt.Errorf("%T is not supported, IncludeResourceRule is expected", value)
+			}
+
+			if len(rule.Fields) == 0 {
+				return validateInclude(rule)
+			}
+
+			return validateFields(rule)
+		}))),
 	)
 }
 
+func validateInclude(rule params.IncludeResourceRule) error {
+	return validation.OneOf("service_offering", "space.organization").
+		Validate(strings.Join(rule.RelationshipPath, "."))
+}
+
+func validateFields(rule params.IncludeResourceRule) error {
+	if strings.Join(rule.RelationshipPath, ".") != "service_offering.service_broker" {
+		return jellidation.NewError("invalid_fields_param", "must be fields[service_offering.service_broker]")
+	}
+
+	return jellidation.Each(validation.OneOf("guid", "name")).Validate(rule.Fields)
+}
+
 func (l *ServicePlanList) ToMessage() repositories.ListServicePlanMessage {
 	return repositories.ListServicePlanMessage{
 		ServiceOfferingGUIDs: parse.ArrayParam(l.ServiceOfferingGUIDs),

diff --git a/api/payloads/service_plan_test.go b/api/payloads/service_plan_test.go
@@ -26,11 +26,17 @@ var _ = Describe("ServicePlan", func() {
 			Entry("available", "available=true", payloads.ServicePlanList{Available: tools.PtrTo(true)}),
 			Entry("not available", "available=false", payloads.ServicePlanList{Available: tools.PtrTo(false)}),
 			Entry("broker names", "service_broker_names=b1,b2", payloads.ServicePlanList{BrokerNames: "b1,b2"}),
-			Entry("include service offering", "include=service_offering", payloads.ServicePlanList{
-				IncludeResourceRules: []params.IncludeResourceRule{{
-					RelationshipPath: []string{"service_offering"},
-					Fields:           []string{},
-				}},
+			Entry("include", "include=service_offering&include=space.organization", payloads.ServicePlanList{
+				IncludeResourceRules: []params.IncludeResourceRule{
+					{
+						RelationshipPath: []string{"service_offering"},
+						Fields:           []string{},
+					},
+					{
+						RelationshipPath: []string{"space", "organization"},
+						Fields:           []string{},
+					},
+				},
 			}),
 			Entry("service broker fields", "fields[service_offering.service_broker]=guid,name", payloads.ServicePlanList{
 				IncludeResourceRules: []params.IncludeResourceRule{{
@@ -41,12 +47,12 @@ var _ = Describe("ServicePlan", func() {
 		)
 
 		DescribeTable("invalid query",
-			func(query string, errMatcher types.GomegaMatcher) {
+			func(query string, matchError types.GomegaMatcher) {
 				_, decodeErr := decodeQuery[payloads.ServicePlanList](query)
-				Expect(decodeErr).To(errMatcher)
+				Expect(decodeErr).To(matchError)
 			},
 			Entry("invalid available", "available=invalid", MatchError(ContainSubstring("failed to parse"))),
-			Entry("invalid include", "include=foo", MatchError(ContainSubstring("value must be one of: service_offering"))),
+			Entry("invalid include", "include=foo", MatchError(ContainSubstring("value must be one of"))),
 			Entry("invalid service broker fields", "fields[service_offering.service_broker]=foo", MatchError(ContainSubstring("value must be one of"))),
 		)