From 53393412c7a795aa0e29f33fd5eb377b28b4f0ce Mon Sep 17 00:00:00 2001 From: Kieron Browne Date: Fri, 23 Jun 2023 11:15:47 +0100 Subject: [PATCH 1/5] Use request validator in `/v3/processes` Co-authored-by: Danail Branekov --- api/handlers/process.go | 16 ++--- api/handlers/process_test.go | 115 +++++++++++++++++++---------------- api/payloads/process.go | 15 ++++- api/payloads/process_test.go | 6 +- 4 files changed, 82 insertions(+), 70 deletions(-) diff --git a/api/handlers/process.go b/api/handlers/process.go index ae24d6556..99b478012 100644 --- a/api/handlers/process.go +++ b/api/handlers/process.go @@ -44,20 +44,20 @@ type Process struct { serverURL url.URL processRepo CFProcessRepository processStats ProcessStats - decoderValidator *DecoderValidator + requestValidator RequestValidator } func NewProcess( serverURL url.URL, processRepo CFProcessRepository, processStatsFetcher ProcessStats, - decoderValidator *DecoderValidator, + requestValidator RequestValidator, ) *Process { return &Process{ serverURL: serverURL, processRepo: processRepo, processStats: processStatsFetcher, - decoderValidator: decoderValidator, + requestValidator: requestValidator, } } @@ -110,7 +110,7 @@ func (h *Process) scale(r *http.Request) (*routing.Response, error) { processGUID := routing.URLParam(r, "guid") var payload payloads.ProcessScale - if err := h.decoderValidator.DecodeAndValidateJSONPayload(r, &payload); err != nil { + if err := h.requestValidator.DecodeAndValidateJSONPayload(r, &payload); err != nil { return nil, apierrors.LogAndReturn(logger, err, "failed to decode payload") } @@ -149,12 +149,8 @@ func (h *Process) list(r *http.Request) (*routing.Response, error) { //nolint:du authInfo, _ := authorization.InfoFromContext(r.Context()) logger := logr.FromContextOrDiscard(r.Context()).WithName("handlers.process.list") - if err := r.ParseForm(); err != nil { - return nil, apierrors.LogAndReturn(logger, err, "Unable to parse request query parameters") - } - processListFilter := new(payloads.ProcessList) - err := payloads.Decode(processListFilter, r.Form) + err := h.requestValidator.DecodeAndValidateURLValues(r, processListFilter) if err != nil { return nil, apierrors.LogAndReturn(logger, err, "Unable to decode request query parameters") } @@ -174,7 +170,7 @@ func (h *Process) update(r *http.Request) (*routing.Response, error) { processGUID := routing.URLParam(r, "guid") var payload payloads.ProcessPatch - if err := h.decoderValidator.DecodeAndValidateJSONPayload(r, &payload); err != nil { + if err := h.requestValidator.DecodeAndValidateJSONPayload(r, &payload); err != nil { return nil, apierrors.LogAndReturn(logger, err, "failed to decode json payload") } diff --git a/api/handlers/process_test.go b/api/handlers/process_test.go index e673a912a..efdc5ed2a 100644 --- a/api/handlers/process_test.go +++ b/api/handlers/process_test.go @@ -2,14 +2,15 @@ package handlers_test import ( "errors" + "io" "net/http" - "net/http/httptest" "strings" "code.cloudfoundry.org/korifi/api/actions" apierrors "code.cloudfoundry.org/korifi/api/errors" . "code.cloudfoundry.org/korifi/api/handlers" "code.cloudfoundry.org/korifi/api/handlers/fake" + "code.cloudfoundry.org/korifi/api/payloads" "code.cloudfoundry.org/korifi/api/repositories" . "code.cloudfoundry.org/korifi/tests/matchers" "code.cloudfoundry.org/korifi/tools" @@ -20,21 +21,21 @@ import ( var _ = Describe("Process", func() { var ( - processRepo *fake.CFProcessRepository - processStats *fake.ProcessStats + processRepo *fake.CFProcessRepository + processStats *fake.ProcessStats + requestValidator *fake.RequestValidator ) BeforeEach(func() { processRepo = new(fake.CFProcessRepository) processStats = new(fake.ProcessStats) - decoderValidator, err := NewDefaultDecoderValidator() - Expect(err).NotTo(HaveOccurred()) + requestValidator = new(fake.RequestValidator) apiHandler := NewProcess( *serverURL, processRepo, processStats, - decoderValidator, + requestValidator, ) routerBuilder.LoadRoutes(apiHandler) }) @@ -151,6 +152,12 @@ var _ = Describe("Process", func() { "memory_in_mb": 512, "disk_in_mb": 256 }` + + requestValidator.DecodeAndValidateJSONPayloadStub = decodeAndValidateJSONPayloadStub(&payloads.ProcessScale{ + Instances: tools.PtrTo(3), + MemoryMB: tools.PtrTo[int64](512), + DiskMB: tools.PtrTo[int64](256), + }) }) JustBeforeEach(func() { @@ -160,6 +167,12 @@ var _ = Describe("Process", func() { }) It("scales the process", func() { + Expect(requestValidator.DecodeAndValidateJSONPayloadCallCount()).To(Equal(1)) + req, _ := requestValidator.DecodeAndValidateJSONPayloadArgsForCall(0) + reqBytes, err := io.ReadAll(req.Body) + Expect(err).NotTo(HaveOccurred()) + Expect(string(reqBytes)).To(Equal(requestBody)) + Expect(processRepo.GetProcessCallCount()).To(Equal(1)) _, actualAuthInfo, actualProcessGUID := processRepo.GetProcessArgsForCall(0) Expect(actualAuthInfo).To(Equal(authInfo)) @@ -190,11 +203,11 @@ var _ = Describe("Process", func() { When("the request JSON is invalid", func() { BeforeEach(func() { - requestBody = `}` + requestValidator.DecodeAndValidateJSONPayloadReturns(errors.New("boom")) }) - It("has the expected error response body", func() { - expectBadRequestError() + It("returns an error", func() { + expectUnknownError() }) }) @@ -227,24 +240,6 @@ var _ = Describe("Process", func() { expectUnknownError() }) }) - - When("validating scale parameters", func() { - DescribeTable("returns a validation decision", - func(requestBody string, status int) { - tableTestRecorder := httptest.NewRecorder() - req, err := http.NewRequestWithContext(ctx, "POST", "/v3/processes/process-guid/actions/scale", strings.NewReader(requestBody)) - Expect(err).NotTo(HaveOccurred()) - routerBuilder.Build().ServeHTTP(tableTestRecorder, req) - Expect(tableTestRecorder.Code).To(Equal(status)) - }, - Entry("instances is negative", `{"instances":-1}`, http.StatusUnprocessableEntity), - Entry("memory is not a positive integer", `{"memory_in_mb":0}`, http.StatusUnprocessableEntity), - Entry("disk is not a positive integer", `{"disk_in_mb":0}`, http.StatusUnprocessableEntity), - Entry("instances is zero", `{"instances":0}`, http.StatusOK), - Entry("memory is a positive integer", `{"memory_in_mb":1024}`, http.StatusOK), - Entry("disk is a positive integer", `{"disk_in_mb":1024}`, http.StatusOK), - ) - }) }) Describe("the GET /v3/processes//stats endpoint", func() { @@ -308,10 +303,8 @@ var _ = Describe("Process", func() { }) Describe("the GET /v3/processes endpoint", func() { - var queryString string - BeforeEach(func() { - queryString = "" + requestValidator.DecodeAndValidateURLValuesStub = decodeAndValidateURLValuesStub(&payloads.ProcessList{}) processRepo.ListProcessesReturns([]repositories.ProcessRecord{ { GUID: "process-guid", @@ -320,12 +313,16 @@ var _ = Describe("Process", func() { }) JustBeforeEach(func() { - req, err := http.NewRequestWithContext(ctx, "GET", "/v3/processes"+queryString, nil) + req, err := http.NewRequestWithContext(ctx, "GET", "/v3/processes", nil) Expect(err).NotTo(HaveOccurred()) routerBuilder.Build().ServeHTTP(rr, req) }) It("returns the processes", func() { + Expect(requestValidator.DecodeAndValidateURLValuesCallCount()).To(Equal(1)) + req, _ := requestValidator.DecodeAndValidateURLValuesArgsForCall(0) + Expect(req.URL.String()).To(HaveSuffix("/v3/processes")) + Expect(rr).To(HaveHTTPStatus(http.StatusOK)) Expect(rr).To(HaveHTTPHeaderWithValue("Content-Type", "application/json")) Expect(rr).To(HaveHTTPBody(SatisfyAll( @@ -335,9 +332,11 @@ var _ = Describe("Process", func() { ))) }) - When("Query Parameters are provided", func() { + When("app_guids query parameter is provided", func() { BeforeEach(func() { - queryString = "?app_guids=my-app-guid" + requestValidator.DecodeAndValidateURLValuesStub = decodeAndValidateURLValuesStub(&payloads.ProcessList{ + AppGUIDs: "my-app-guid", + }) }) It("invokes process repository with correct args", func() { @@ -349,13 +348,13 @@ var _ = Describe("Process", func() { }) }) - When("invalid query parameters are provided", func() { + When("the request body is invalid", func() { BeforeEach(func() { - queryString = "?foo=my-app-guid" + requestValidator.DecodeAndValidateURLValuesReturns(errors.New("boo")) }) - It("returns an Unknown key error", func() { - expectUnknownKeyError("The query parameter is invalid: Valid parameters are: .*") + It("returns an error", func() { + expectUnknownError() }) }) @@ -397,6 +396,22 @@ var _ = Describe("Process", func() { processRepo.PatchProcessReturns(repositories.ProcessRecord{ GUID: "process-guid", }, nil) + + requestValidator.DecodeAndValidateJSONPayloadStub = decodeAndValidateJSONPayloadStub(&payloads.ProcessPatch{ + Metadata: &payloads.MetadataPatch{ + Labels: map[string]*string{ + "foo": tools.PtrTo("value1"), + }, + }, + HealthCheck: &payloads.HealthCheck{ + Type: tools.PtrTo("port"), + Data: &payloads.Data{ + Timeout: tools.PtrTo[int64](5), + Endpoint: tools.PtrTo("http://myapp.com/health"), + InvocationTimeout: tools.PtrTo[int64](2), + }, + }, + }) }) JustBeforeEach(func() { @@ -406,6 +421,12 @@ var _ = Describe("Process", func() { }) It("updates the process", func() { + Expect(requestValidator.DecodeAndValidateJSONPayloadCallCount()).To(Equal(1)) + req, _ := requestValidator.DecodeAndValidateJSONPayloadArgsForCall(0) + reqBytes, err := io.ReadAll(req.Body) + Expect(err).NotTo(HaveOccurred()) + Expect(string(reqBytes)).To(Equal(requestBody)) + Expect(processRepo.PatchProcessCallCount()).To(Equal(1)) _, actualAuthInfo, actualMsg := processRepo.PatchProcessArgsForCall(0) Expect(actualAuthInfo).To(Equal(authInfo)) @@ -426,25 +447,11 @@ var _ = Describe("Process", func() { When("the request body is invalid json", func() { BeforeEach(func() { - requestBody = `{` + requestValidator.DecodeAndValidateJSONPayloadReturns(errors.New("boom")) }) - It("return an request malformed error", func() { - expectBadRequestError() - }) - }) - - When("the request body is invalid with an unknown field", func() { - BeforeEach(func() { - requestBody = `{ - "health_check": { - "endpoint": "my-endpoint" - } - }` - }) - - It("return an request malformed error", func() { - expectUnprocessableEntityError("invalid request body: json: unknown field \"endpoint\"") + It("return an error", func() { + expectUnknownError() }) }) diff --git a/api/payloads/process.go b/api/payloads/process.go index 05eaf7842..5e0b35e30 100644 --- a/api/payloads/process.go +++ b/api/payloads/process.go @@ -5,12 +5,21 @@ import ( "code.cloudfoundry.org/korifi/api/payloads/parse" "code.cloudfoundry.org/korifi/api/repositories" + "github.com/jellydator/validation" ) type ProcessScale struct { - Instances *int `json:"instances" validate:"omitempty,gte=0"` - MemoryMB *int64 `json:"memory_in_mb" validate:"omitempty,gt=0"` - DiskMB *int64 `json:"disk_in_mb" validate:"omitempty,gt=0"` + Instances *int `json:"instances"` + MemoryMB *int64 `json:"memory_in_mb"` + DiskMB *int64 `json:"disk_in_mb"` +} + +func (p ProcessScale) Validate() error { + return validation.ValidateStruct(&p, + validation.Field(&p.Instances, validation.Min(0).Error("must be 0 or greater")), + validation.Field(&p.MemoryMB, validation.Min(1).Error("must be greater than 0")), + validation.Field(&p.DiskMB, validation.Min(1).Error("must be greater than 0")), + ) } type ProcessPatch struct { diff --git a/api/payloads/process_test.go b/api/payloads/process_test.go index cc40854c4..ea4e54250 100644 --- a/api/payloads/process_test.go +++ b/api/payloads/process_test.go @@ -70,7 +70,7 @@ var _ = Describe("Process payload validation", func() { }) It("returns an error", func() { - expectUnprocessableEntityError(validatorErr, "Instances must be 0 or greater") + expectUnprocessableEntityError(validatorErr, "instances must be 0 or greater") }) }) @@ -80,7 +80,7 @@ var _ = Describe("Process payload validation", func() { }) It("returns an error", func() { - expectUnprocessableEntityError(validatorErr, "MemoryMB must be greater than 0") + expectUnprocessableEntityError(validatorErr, "memory_in_mb must be greater than 0") }) }) @@ -90,7 +90,7 @@ var _ = Describe("Process payload validation", func() { }) It("returns an error", func() { - expectUnprocessableEntityError(validatorErr, "DiskMB must be greater than 0") + expectUnprocessableEntityError(validatorErr, "disk_in_mb must be greater than 0") }) }) }) From d4fffb590581de4abbeef6534a1b37fcb89b8f13 Mon Sep 17 00:00:00 2001 From: Kieron Browne Date: Fri, 23 Jun 2023 15:49:59 +0100 Subject: [PATCH 2/5] Move role api param testing to payloads package We also migrated all role validation to jellydator Issue: https://github.com/cloudfoundry/korifi/issues/1996 Co-authored-by: Danail Branekov --- api/handlers/role.go | 36 ++------- api/handlers/role_test.go | 74 +++++++++--------- api/handlers/validator.go | 60 --------------- api/payloads/role.go | 126 ++++++++++++++++++++++++------- api/payloads/role_test.go | 121 ++++++++++++++--------------- api/payloads/validation/rules.go | 2 +- 6 files changed, 199 insertions(+), 220 deletions(-) diff --git a/api/handlers/role.go b/api/handlers/role.go index 1a55b8ad2..13011a895 100644 --- a/api/handlers/role.go +++ b/api/handlers/role.go @@ -21,22 +21,6 @@ const ( RolePath = RolesPath + "/{guid}" ) -type RoleName string - -const ( - RoleAdmin RoleName = "admin" - RoleAdminReadOnly RoleName = "admin_read_only" - RoleGlobalAuditor RoleName = "global_auditor" - RoleOrganizationAuditor RoleName = "organization_auditor" - RoleOrganizationBillingManager RoleName = "organization_billing_manager" - RoleOrganizationManager RoleName = "organization_manager" - RoleOrganizationUser RoleName = "organization_user" - RoleSpaceAuditor RoleName = "space_auditor" - RoleSpaceDeveloper RoleName = "space_developer" - RoleSpaceManager RoleName = "space_manager" - RoleSpaceSupporter RoleName = "space_supporter" -) - //counterfeiter:generate -o fake -fake-name CFRoleRepository . CFRoleRepository type CFRoleRepository interface { @@ -49,14 +33,14 @@ type CFRoleRepository interface { type Role struct { apiBaseURL url.URL roleRepo CFRoleRepository - decoderValidator RequestValidator + requestValidator RequestValidator } -func NewRole(apiBaseURL url.URL, roleRepo CFRoleRepository, decoderValidator RequestValidator) *Role { +func NewRole(apiBaseURL url.URL, roleRepo CFRoleRepository, requestValidator RequestValidator) *Role { return &Role{ apiBaseURL: apiBaseURL, roleRepo: roleRepo, - decoderValidator: decoderValidator, + requestValidator: requestValidator, } } @@ -65,7 +49,7 @@ func (h *Role) create(r *http.Request) (*routing.Response, error) { logger := logr.FromContextOrDiscard(r.Context()).WithName("handlers.role.create") var payload payloads.RoleCreate - if err := h.decoderValidator.DecodeAndValidateJSONPayload(r, &payload); err != nil { + if err := h.requestValidator.DecodeAndValidateJSONPayload(r, &payload); err != nil { return nil, apierrors.LogAndReturn(logger, err, "failed to decode payload") } @@ -84,12 +68,8 @@ func (h *Role) list(r *http.Request) (*routing.Response, error) { authInfo, _ := authorization.InfoFromContext(r.Context()) logger := logr.FromContextOrDiscard(r.Context()).WithName("handlers.role.list") - if err := r.ParseForm(); err != nil { - return nil, apierrors.LogAndReturn(logger, err, "Unable to parse request query parameters") - } - - roleListFilter := new(payloads.RoleListFilter) - err := payloads.Decode(roleListFilter, r.Form) + roleListFilter := new(payloads.RoleList) + err := h.requestValidator.DecodeAndValidateURLValues(r, roleListFilter) if err != nil { return nil, apierrors.LogAndReturn(logger, err, "Unable to decode request query parameters") } @@ -101,14 +81,14 @@ func (h *Role) list(r *http.Request) (*routing.Response, error) { filteredRoles := filterRoles(roleListFilter, roles) - if err := h.sortList(filteredRoles, r.FormValue("order_by")); err != nil { + if err := h.sortList(filteredRoles, roleListFilter.OrderBy); err != nil { return nil, apierrors.LogAndReturn(logger, err, "unable to parse order by request") } return routing.NewResponse(http.StatusOK).WithBody(presenter.ForList(presenter.ForRole, filteredRoles, h.apiBaseURL, *r.URL)), nil } -func filterRoles(roleListFilter *payloads.RoleListFilter, roles []repositories.RoleRecord) []repositories.RoleRecord { +func filterRoles(roleListFilter *payloads.RoleList, roles []repositories.RoleRecord) []repositories.RoleRecord { var filteredRoles []repositories.RoleRecord for _, role := range roles { if match(roleListFilter.GUIDs, role.GUID) && diff --git a/api/handlers/role_test.go b/api/handlers/role_test.go index c50d50864..bebf128ff 100644 --- a/api/handlers/role_test.go +++ b/api/handlers/role_test.go @@ -2,6 +2,7 @@ package handlers_test import ( "errors" + "io" "net/http" "net/http/httptest" "strings" @@ -45,7 +46,7 @@ var _ = Describe("Role", func() { roleCreate = &payloads.RoleCreate{ Type: "space_developer", Relationships: payloads.RoleRelationships{ - User: &payloads.UserRelationship{ + User: payloads.UserRelationship{ Data: payloads.UserRelationshipData{ Username: "my-user", }, @@ -62,12 +63,18 @@ var _ = Describe("Role", func() { }) JustBeforeEach(func() { - req, err := http.NewRequestWithContext(ctx, "POST", rolesBase, strings.NewReader("")) + req, err := http.NewRequestWithContext(ctx, "POST", rolesBase, strings.NewReader("payload-data")) Expect(err).NotTo(HaveOccurred()) routerBuilder.Build().ServeHTTP(rr, req) }) It("creates the role", func() { + Expect(requestValidator.DecodeAndValidateJSONPayloadCallCount()).To(Equal(1)) + req, _ := requestValidator.DecodeAndValidateJSONPayloadArgsForCall(0) + bodyBytes, err := io.ReadAll(req.Body) + Expect(err).NotTo(HaveOccurred()) + Expect(string(bodyBytes)).To(Equal("payload-data")) + Expect(roleRepo.CreateRoleCallCount()).To(Equal(1)) _, actualAuthInfo, roleMessage := roleRepo.CreateRoleArgsForCall(0) Expect(actualAuthInfo).To(Equal(authInfo)) @@ -153,23 +160,25 @@ var _ = Describe("Role", func() { }) Describe("List roles", func() { - var query string - BeforeEach(func() { - query = "" roleRepo.ListRolesReturns([]repositories.RoleRecord{ {GUID: "role-1"}, {GUID: "role-2"}, }, nil) + requestValidator.DecodeAndValidateURLValuesStub = decodeAndValidateURLValuesStub(&payloads.RoleList{}) }) JustBeforeEach(func() { - req, err := http.NewRequestWithContext(ctx, "GET", rolesBase+query, nil) + req, err := http.NewRequestWithContext(ctx, "GET", rolesBase+"?foo=bar", nil) Expect(err).NotTo(HaveOccurred()) routerBuilder.Build().ServeHTTP(rr, req) }) It("lists roles", func() { + Expect(requestValidator.DecodeAndValidateURLValuesCallCount()).To(Equal(1)) + req, _ := requestValidator.DecodeAndValidateURLValuesArgsForCall(0) + Expect(req.URL.String()).To(HaveSuffix(rolesBase + "?foo=bar")) + Expect(roleRepo.ListRolesCallCount()).To(Equal(1)) _, actualAuthInfo := roleRepo.ListRolesArgsForCall(0) Expect(actualAuthInfo).To(Equal(authInfo)) @@ -178,7 +187,7 @@ var _ = Describe("Role", func() { Expect(rr).To(HaveHTTPHeaderWithValue("Content-Type", "application/json")) Expect(rr).To(HaveHTTPBody(SatisfyAll( MatchJSONPath("$.pagination.total_results", BeEquivalentTo(2)), - MatchJSONPath("$.pagination.first.href", "https://api.example.org/v3/roles"), + MatchJSONPath("$.pagination.first.href", "https://api.example.org/v3/roles?foo=bar"), MatchJSONPath("$.resources", HaveLen(2)), MatchJSONPath("$.resources[0].guid", "role-1"), MatchJSONPath("$.resources[0].links.self.href", "https://api.example.org/v3/roles/role-1"), @@ -186,17 +195,6 @@ var _ = Describe("Role", func() { ))) }) - When("include is specified", func() { - BeforeEach(func() { - query = "?include=user" - }) - - It("does not fail but has no effect on the result", func() { - Expect(rr).To(HaveHTTPStatus(http.StatusOK)) - Expect(rr).To(HaveHTTPBody(MatchJSONPath("$.pagination.total_results", BeEquivalentTo(2)))) - }) - }) - Describe("filtering and ordering", func() { BeforeEach(func() { roleRepo.ListRolesReturns([]repositories.RoleRecord{ @@ -207,31 +205,35 @@ var _ = Describe("Role", func() { }, nil) }) - DescribeTable("filtering", func(filter string, expectedGUIDs ...any) { - req, err := http.NewRequestWithContext(ctx, "GET", rolesBase+"?"+filter, nil) + DescribeTable("filtering", func(filter payloads.RoleList, expectedGUIDs ...any) { + req, err := http.NewRequestWithContext(ctx, "GET", rolesBase+"?foo=bar", nil) Expect(err).NotTo(HaveOccurred()) + + requestValidator.DecodeAndValidateURLValuesStub = decodeAndValidateURLValuesStub(&filter) rr = httptest.NewRecorder() routerBuilder.Build().ServeHTTP(rr, req) Expect(rr).To(HaveHTTPStatus(http.StatusOK)) Expect(rr).To(HaveHTTPBody(MatchJSONPath("$.resources[*].guid", expectedGUIDs))) }, - Entry("no filter", "", "1", "2", "3", "4"), - Entry("guids1", "guids=4", "4"), - Entry("guids2", "guids=1,3", "1", "3"), - Entry("types1", "types=a", "1"), - Entry("types2", "types=b,c", "2", "3", "4"), - Entry("space_guids1", "space_guids=space1", "1"), - Entry("space_guids2", "space_guids=space1,space2", "1", "2"), - Entry("organization_guids1", "organization_guids=org1", "3"), - Entry("organization_guids2", "organization_guids=org1,org2", "3", "4"), - Entry("user_guids1", "user_guids=user1", "1", "2", "3"), - Entry("user_guids2", "user_guids=user1,user2", "1", "2", "3", "4"), + Entry("no filter", payloads.RoleList{}, "1", "2", "3", "4"), + Entry("guids1", payloads.RoleList{GUIDs: map[string]bool{"4": true}}, "4"), + Entry("guids2", payloads.RoleList{GUIDs: map[string]bool{"1": true, "3": true}}, "1", "3"), + Entry("types1", payloads.RoleList{Types: map[string]bool{"a": true}}, "1"), + Entry("types2", payloads.RoleList{Types: map[string]bool{"b": true, "c": true}}, "2", "3", "4"), + Entry("space_guids1", payloads.RoleList{SpaceGUIDs: map[string]bool{"space1": true}}, "1"), + Entry("space_guids2", payloads.RoleList{SpaceGUIDs: map[string]bool{"space1": true, "space2": true}}, "1", "2"), + Entry("organization_guids1", payloads.RoleList{OrgGUIDs: map[string]bool{"org1": true}}, "3"), + Entry("organization_guids2", payloads.RoleList{OrgGUIDs: map[string]bool{"org1": true, "org2": true}}, "3", "4"), + Entry("user_guids1", payloads.RoleList{UserGUIDs: map[string]bool{"user1": true}}, "1", "2", "3"), + Entry("user_guids2", payloads.RoleList{UserGUIDs: map[string]bool{"user1": true, "user2": true}}, "1", "2", "3", "4"), ) DescribeTable("ordering", func(order string, expectedGUIDs ...any) { - req, err := http.NewRequestWithContext(ctx, "GET", rolesBase+"?order_by="+order, nil) + req, err := http.NewRequestWithContext(ctx, "GET", rolesBase+"?order_by=not-used-in-test", nil) Expect(err).NotTo(HaveOccurred()) + + requestValidator.DecodeAndValidateURLValuesStub = decodeAndValidateURLValuesStub(&payloads.RoleList{OrderBy: order}) rr = httptest.NewRecorder() routerBuilder.Build().ServeHTTP(rr, req) @@ -245,13 +247,13 @@ var _ = Describe("Role", func() { ) }) - When("order_by is not a valid field", func() { + When("decoding the url values fails", func() { BeforeEach(func() { - query = "?order_by=not_valid" + requestValidator.DecodeAndValidateURLValuesReturns(errors.New("boom")) }) - It("returns an Unknown key error", func() { - expectUnknownKeyError("The query parameter is invalid: Order by can only be: .*") + It("returns an Unknown error", func() { + expectUnknownError() }) }) diff --git a/api/handlers/validator.go b/api/handlers/validator.go index ccb46a1ac..26819bfc2 100644 --- a/api/handlers/validator.go +++ b/api/handlers/validator.go @@ -11,7 +11,6 @@ import ( "strings" apierrors "code.cloudfoundry.org/korifi/api/errors" - "code.cloudfoundry.org/korifi/api/payloads" "github.com/go-playground/locales/en" ut "github.com/go-playground/universal-translator" @@ -227,28 +226,6 @@ func wireValidator() (*validator.Validate, ut.Translator, error) { return nil, nil, err } - v.RegisterStructValidation(checkRoleTypeAndOrgSpace, payloads.RoleCreate{}) - - err = v.RegisterTranslation("cannot_have_both_org_and_space_set", trans, func(ut ut.Translator) error { - return ut.Add("cannot_have_both_org_and_space_set", "Cannot pass both 'organization' and 'space' in a create role request", false) - }, func(ut ut.Translator, fe validator.FieldError) string { - t, _ := ut.T("cannot_have_both_org_and_space_set", fe.Field()) - return t - }) - if err != nil { - return nil, nil, err - } - - err = v.RegisterTranslation("valid_role", trans, func(ut ut.Translator) error { - return ut.Add("valid_role", "{0} is not a valid role", false) - }, func(ut ut.Translator, fe validator.FieldError) string { - t, _ := ut.T("valid_role", fmt.Sprintf("%v", fe.Value())) - return t - }) - if err != nil { - return nil, nil, err - } - return v, trans, nil } @@ -265,43 +242,6 @@ func registerDefaultTranslator(v *validator.Validate) (ut.Translator, error) { return trans, nil } -func checkRoleTypeAndOrgSpace(sl validator.StructLevel) { - roleCreate := sl.Current().Interface().(payloads.RoleCreate) - - if roleCreate.Relationships.Organization != nil && roleCreate.Relationships.Space != nil { - sl.ReportError(roleCreate.Relationships.Organization, "relationships.organization", "Organization", "cannot_have_both_org_and_space_set", "") - } - - roleType := RoleName(roleCreate.Type) - - switch roleType { - case RoleSpaceManager: - fallthrough - case RoleSpaceAuditor: - fallthrough - case RoleSpaceDeveloper: - fallthrough - case RoleSpaceSupporter: - if roleCreate.Relationships.Space == nil { - sl.ReportError(roleCreate.Relationships.Space, "relationships.space", "Space", "required", "") - } - case RoleOrganizationUser: - fallthrough - case RoleOrganizationAuditor: - fallthrough - case RoleOrganizationManager: - fallthrough - case RoleOrganizationBillingManager: - if roleCreate.Relationships.Organization == nil { - sl.ReportError(roleCreate.Relationships.Organization, "relationships.organization", "Organization", "required", "") - } - - case RoleName(""): - default: - sl.ReportError(roleCreate.Type, "type", "Role type", "valid_role", "") - } -} - func serviceInstanceTagLength(fl validator.FieldLevel) bool { tags, ok := fl.Field().Interface().([]string) if !ok { diff --git a/api/payloads/role.go b/api/payloads/role.go index 909a30ca2..35553bfca 100644 --- a/api/payloads/role.go +++ b/api/payloads/role.go @@ -1,44 +1,53 @@ package payloads import ( + "context" "net/url" "strings" "code.cloudfoundry.org/korifi/api/authorization" + "code.cloudfoundry.org/korifi/api/payloads/validation" + jellidation "github.com/jellydator/validation" "code.cloudfoundry.org/korifi/api/repositories" rbacv1 "k8s.io/api/rbac/v1" ) -type ( - RoleCreate struct { - Type string `json:"type" validate:"required"` - Relationships RoleRelationships `json:"relationships" validate:"required"` - } +const ( + RoleAdmin = "admin" + RoleAdminReadOnly = "admin_read_only" + RoleGlobalAuditor = "global_auditor" + RoleOrganizationAuditor = "organization_auditor" + RoleOrganizationBillingManager = "organization_billing_manager" + RoleOrganizationManager = "organization_manager" + RoleOrganizationUser = "organization_user" + RoleSpaceAuditor = "space_auditor" + RoleSpaceDeveloper = "space_developer" + RoleSpaceManager = "space_manager" + RoleSpaceSupporter = "space_supporter" +) - UserRelationship struct { - Data UserRelationshipData `json:"data" validate:"required"` - } +type RoleCreate struct { + Type string `json:"type"` + Relationships RoleRelationships `json:"relationships"` +} - UserRelationshipData struct { - Username string `json:"username" validate:"required_without=GUID"` - GUID string `json:"guid" validate:"required_without=Username"` - } +type ctxType string - RoleRelationships struct { - User *UserRelationship `json:"user" validate:"required"` - Space *Relationship `json:"space"` - Organization *Relationship `json:"organization"` - } +const typeKey ctxType = "type" - RoleListFilter struct { - GUIDs map[string]bool - Types map[string]bool - SpaceGUIDs map[string]bool - OrgGUIDs map[string]bool - UserGUIDs map[string]bool - } -) +func (p RoleCreate) Validate() error { + ctx := context.WithValue(context.Background(), typeKey, p.Type) + + return jellidation.ValidateStructWithContext(ctx, &p, + jellidation.Field(&p.Type, + jellidation.Required, + validation.OneOf(RoleSpaceManager, RoleSpaceAuditor, RoleSpaceDeveloper, RoleSpaceSupporter, + RoleOrganizationUser, RoleOrganizationAuditor, RoleOrganizationManager, RoleOrganizationBillingManager), + ), + jellidation.Field(&p.Relationships, validation.StrictlyRequired), + ) +} func (p RoleCreate) ToMessage() repositories.CreateRoleMessage { record := repositories.CreateRoleMessage{ @@ -70,25 +79,84 @@ func (p RoleCreate) ToMessage() repositories.CreateRoleMessage { return record } -func (r *RoleListFilter) SupportedKeys() []string { +type RoleRelationships struct { + User UserRelationship `json:"user"` + Space *Relationship `json:"space"` + Organization *Relationship `json:"organization"` +} + +func (r RoleRelationships) ValidateWithContext(ctx context.Context) error { + roleType := ctx.Value(typeKey) + + return jellidation.ValidateStruct(&r, + jellidation.Field(&r.User, validation.StrictlyRequired), + + jellidation.Field(&r.Space, + jellidation.When(r.Organization != nil && r.Organization.Data != nil && r.Organization.Data.GUID != "", + jellidation.Nil.Error("cannot pass both 'organization' and 'space' in a create role request"))), + + jellidation.Field(&r.Space, + jellidation.When( + roleType == RoleSpaceAuditor || roleType == RoleSpaceDeveloper || + roleType == RoleSpaceManager || roleType == RoleSpaceSupporter, + jellidation.NotNil, + validation.StrictlyRequired, + )), + + jellidation.Field(&r.Organization, + jellidation.When( + roleType == RoleOrganizationAuditor || roleType == RoleOrganizationBillingManager || + roleType == RoleOrganizationManager || roleType == RoleOrganizationUser, + jellidation.NotNil, + validation.StrictlyRequired, + )), + ) +} + +type UserRelationship struct { + Data UserRelationshipData `json:"data"` +} + +type UserRelationshipData struct { + Username string `json:"username"` + GUID string `json:"guid"` +} + +type RoleList struct { + GUIDs map[string]bool + Types map[string]bool + SpaceGUIDs map[string]bool + OrgGUIDs map[string]bool + UserGUIDs map[string]bool + OrderBy string +} + +func (r RoleList) SupportedKeys() []string { return []string{"guids", "types", "space_guids", "organization_guids", "user_guids", "order_by", "include", "per_page", "page"} } -func (r *RoleListFilter) DecodeFromURLValues(values url.Values) error { +func (r *RoleList) DecodeFromURLValues(values url.Values) error { r.GUIDs = commaSepToSet(values.Get("guids")) r.Types = commaSepToSet(values.Get("types")) r.SpaceGUIDs = commaSepToSet(values.Get("space_guids")) r.OrgGUIDs = commaSepToSet(values.Get("organization_guids")) r.UserGUIDs = commaSepToSet(values.Get("user_guids")) + r.OrderBy = values.Get("order_by") return nil } +func (r RoleList) Validate() error { + return jellidation.ValidateStruct(&r, + jellidation.Field(&r.OrderBy, validation.OneOf("created_at", "updated_at", "-created_at", "-updated_at")), + ) +} + func commaSepToSet(in string) map[string]bool { - out := map[string]bool{} if in == "" { - return out + return nil } + out := map[string]bool{} for _, s := range strings.Split(in, ",") { out[s] = true } diff --git a/api/payloads/role_test.go b/api/payloads/role_test.go index dcf75c3ce..1783d0a92 100644 --- a/api/payloads/role_test.go +++ b/api/payloads/role_test.go @@ -8,7 +8,6 @@ import ( rbacv1 "k8s.io/api/rbac/v1" "code.cloudfoundry.org/korifi/api/errors" - "code.cloudfoundry.org/korifi/api/handlers" "code.cloudfoundry.org/korifi/api/payloads" . "github.com/onsi/ginkgo/v2" @@ -29,7 +28,7 @@ var _ = Describe("RoleCreate", func() { createPayload = payloads.RoleCreate{ Type: "space_manager", Relationships: payloads.RoleRelationships{ - User: &payloads.UserRelationship{ + User: payloads.UserRelationship{ Data: payloads.UserRelationshipData{ Username: "cf-service-account", }, @@ -53,33 +52,15 @@ var _ = Describe("RoleCreate", func() { Expect(roleCreate).To(PointTo(Equal(createPayload))) }) - When("the user name is missing", func() { - BeforeEach(func() { - createPayload.Relationships.User = nil - }) - - It("fails", func() { - Expect(apiError).To(HaveOccurred()) - Expect(apiError.Detail()).To(ContainSubstring("User is a required field")) - }) - }) - When("the user name and GUID are missing", func() { BeforeEach(func() { - createPayload.Relationships.User = &payloads.UserRelationship{ - Data: payloads.UserRelationshipData{ - Username: "", - GUID: "", - }, - } + createPayload.Relationships.User.Data.GUID = "" + createPayload.Relationships.User.Data.Username = "" }) It("fails", func() { Expect(apiError).To(HaveOccurred()) - Expect(apiError.Detail()).To(SatisfyAll( - ContainSubstring("Field validation for 'GUID' failed"), - ContainSubstring("Field validation for 'Username' failed"), - )) + Expect(apiError.Detail()).To(ContainSubstring("user cannot be blank")) }) }) @@ -90,29 +71,7 @@ var _ = Describe("RoleCreate", func() { It("fails", func() { Expect(apiError).To(HaveOccurred()) - Expect(apiError.Detail()).To(ContainSubstring("Type is a required field")) - }) - }) - - When("a space role is missing a space relationship", func() { - BeforeEach(func() { - createPayload.Relationships.Space = nil - }) - - It("fails", func() { - Expect(apiError).To(HaveOccurred()) - Expect(apiError.Detail()).To(ContainSubstring("relationships.space is a required field")) - }) - }) - - When("an org role is missing an org relationship", func() { - BeforeEach(func() { - createPayload.Type = "organization_manager" - }) - - It("fails", func() { - Expect(apiError).To(HaveOccurred()) - Expect(apiError.Detail()).To(ContainSubstring("relationships.organization is a required field")) + Expect(apiError.Detail()).To(ContainSubstring("type cannot be blank")) }) }) @@ -127,7 +86,7 @@ var _ = Describe("RoleCreate", func() { It("fails", func() { Expect(apiError).To(HaveOccurred()) - Expect(apiError.Detail()).To(ContainSubstring("Cannot pass both 'organization' and 'space' in a create role request")) + Expect(apiError.Detail()).To(ContainSubstring("cannot pass both 'organization' and 'space' in a create role request")) }) }) @@ -198,23 +157,53 @@ var _ = DescribeTable("Role org / space combination validation", } }, - Entry("org auditor w org", string(handlers.RoleOrganizationAuditor), "organization", true, ""), - Entry("org auditor w space", string(handlers.RoleOrganizationAuditor), "space", false, "relationships.organization is a required field"), - Entry("org billing manager w org", string(handlers.RoleOrganizationBillingManager), "organization", true, ""), - Entry("org billing manager w space", string(handlers.RoleOrganizationBillingManager), "space", false, "relationships.organization is a required field"), - Entry("org manager w org", string(handlers.RoleOrganizationManager), "organization", true, ""), - Entry("org manager w space", string(handlers.RoleOrganizationManager), "space", false, "relationships.organization is a required field"), - Entry("org user w org", string(handlers.RoleOrganizationUser), "organization", true, ""), - Entry("org user w space", string(handlers.RoleOrganizationUser), "space", false, "relationships.organization is a required field"), - - Entry("space auditor w org", string(handlers.RoleSpaceAuditor), "organization", false, "relationships.space is a required field"), - Entry("space auditor w space", string(handlers.RoleSpaceAuditor), "space", true, ""), - Entry("space developer w org", string(handlers.RoleSpaceDeveloper), "organization", false, "relationships.space is a required field"), - Entry("space developer w space", string(handlers.RoleSpaceDeveloper), "space", true, ""), - Entry("space manager w org", string(handlers.RoleSpaceManager), "organization", false, "relationships.space is a required field"), - Entry("space manager w space", string(handlers.RoleSpaceManager), "space", true, ""), - Entry("space supporter w org", string(handlers.RoleSpaceSupporter), "organization", false, "relationships.space is a required field"), - Entry("space supporter w space", string(handlers.RoleSpaceSupporter), "space", true, ""), - - Entry("invalid role name", "does-not-exist", "organization", false, "does-not-exist is not a valid role"), + Entry("org auditor w org", payloads.RoleOrganizationAuditor, "organization", true, ""), + Entry("org auditor w space", payloads.RoleOrganizationAuditor, "space", false, "relationships.organization is required"), + Entry("org billing manager w org", payloads.RoleOrganizationBillingManager, "organization", true, ""), + Entry("org billing manager w space", payloads.RoleOrganizationBillingManager, "space", false, "relationships.organization is required"), + Entry("org manager w org", payloads.RoleOrganizationManager, "organization", true, ""), + Entry("org manager w space", payloads.RoleOrganizationManager, "space", false, "relationships.organization is required"), + Entry("org user w org", payloads.RoleOrganizationUser, "organization", true, ""), + Entry("org user w space", payloads.RoleOrganizationUser, "space", false, "relationships.organization is required"), + + Entry("space auditor w org", payloads.RoleSpaceAuditor, "organization", false, "relationships.space is required"), + Entry("space auditor w space", payloads.RoleSpaceAuditor, "space", true, ""), + Entry("space developer w org", payloads.RoleSpaceDeveloper, "organization", false, "relationships.space is required"), + Entry("space developer w space", payloads.RoleSpaceDeveloper, "space", true, ""), + Entry("space manager w org", payloads.RoleSpaceManager, "organization", false, "relationships.space is required"), + Entry("space manager w space", payloads.RoleSpaceManager, "space", true, ""), + Entry("space supporter w org", payloads.RoleSpaceSupporter, "organization", false, "relationships.space is required"), + Entry("space supporter w space", payloads.RoleSpaceSupporter, "space", true, ""), + + Entry("invalid role name", "does-not-exist", "organization", false, "type value must be one of"), ) + +var _ = Describe("role list", func() { + DescribeTable("valid query", + func(query string, expectedRoleListQueryParameters payloads.RoleList) { + actualRoleListQueryParameters, decodeErr := decodeQuery[payloads.RoleList](query) + + Expect(decodeErr).NotTo(HaveOccurred()) + Expect(*actualRoleListQueryParameters).To(Equal(expectedRoleListQueryParameters)) + }, + + Entry("guids", "guids=g1,g2", payloads.RoleList{GUIDs: map[string]bool{"g1": true, "g2": true}}), + Entry("types", "types=g1,g2", payloads.RoleList{Types: map[string]bool{"g1": true, "g2": true}}), + Entry("space_guids", "space_guids=g1,g2", payloads.RoleList{SpaceGUIDs: map[string]bool{"g1": true, "g2": true}}), + Entry("organization_guids", "organization_guids=g1,g2", payloads.RoleList{OrgGUIDs: map[string]bool{"g1": true, "g2": true}}), + Entry("user_guids", "user_guids=g1,g2", payloads.RoleList{UserGUIDs: map[string]bool{"g1": true, "g2": true}}), + Entry("order_by1", "order_by=created_at", payloads.RoleList{OrderBy: "created_at"}), + Entry("order_by2", "order_by=-created_at", payloads.RoleList{OrderBy: "-created_at"}), + Entry("order_by3", "order_by=updated_at", payloads.RoleList{OrderBy: "updated_at"}), + Entry("order_by4", "order_by=-updated_at", payloads.RoleList{OrderBy: "-updated_at"}), + Entry("include", "include=foo", payloads.RoleList{}), + ) + + DescribeTable("invalid query", + func(query string, expectedErrMsg string) { + _, decodeErr := decodeQuery[payloads.RoleList](query) + Expect(decodeErr).To(MatchError(ContainSubstring(expectedErrMsg))) + }, + Entry("invalid order_by", "order_by=foo", "value must be one of"), + ) +}) diff --git a/api/payloads/validation/rules.go b/api/payloads/validation/rules.go index 3ee11138b..04c558cf6 100644 --- a/api/payloads/validation/rules.go +++ b/api/payloads/validation/rules.go @@ -26,7 +26,7 @@ type strictlyRequiredRule struct { validation.RequiredRule } -// We wrap tje original validation.RequiredRule in order to workaround +// We wrap the original validation.RequiredRule in order to workaround // incorrect zero type check: // https://github.com/jellydator/validation/blob/44595f5c48dd0da8bbeff0f56ceaa581631e55b1/util.go#L151-L156 func (r strictlyRequiredRule) Validate(value interface{}) error { From 3bbc625bb8b7e276be6342326e9dc83fa05ff99b Mon Sep 17 00:00:00 2001 From: Kieron Browne Date: Mon, 26 Jun 2023 16:50:26 +0100 Subject: [PATCH 3/5] Test route handler using fake request validator This includes: - switching list routes to use the requestValidator to parse URL params - changing from go-playground to jellydator validation Issue: https://github.com/cloudfoundry/korifi/issues/1996 --- api/handlers/route.go | 55 +++-- api/handlers/route_test.go | 408 ++++++++++-------------------------- api/payloads/destination.go | 58 ----- api/payloads/route.go | 126 +++++++++-- api/payloads/route_test.go | 248 +++++++++++++++++++++- 5 files changed, 492 insertions(+), 403 deletions(-) delete mode 100644 api/payloads/destination.go diff --git a/api/handlers/route.go b/api/handlers/route.go index a6746c4ff..7c90123ca 100644 --- a/api/handlers/route.go +++ b/api/handlers/route.go @@ -41,7 +41,7 @@ type Route struct { domainRepo CFDomainRepository appRepo CFAppRepository spaceRepo SpaceRepository - decoderValidator *DecoderValidator + requestValidator RequestValidator } func NewRoute( @@ -50,7 +50,7 @@ func NewRoute( domainRepo CFDomainRepository, appRepo CFAppRepository, spaceRepo SpaceRepository, - decoderValidator *DecoderValidator, + requestValidator RequestValidator, ) *Route { return &Route{ serverURL: serverURL, @@ -58,7 +58,7 @@ func NewRoute( domainRepo: domainRepo, appRepo: appRepo, spaceRepo: spaceRepo, - decoderValidator: decoderValidator, + requestValidator: requestValidator, } } @@ -80,13 +80,8 @@ func (h *Route) list(r *http.Request) (*routing.Response, error) { authInfo, _ := authorization.InfoFromContext(r.Context()) logger := logr.FromContextOrDiscard(r.Context()).WithName("handlers.route.list") - if err := r.ParseForm(); err != nil { - return nil, apierrors.LogAndReturn(logger, err, "Unable to parse request query parameters") - } - routeListFilter := new(payloads.RouteList) - err := payloads.Decode(routeListFilter, r.Form) - if err != nil { + if err := h.requestValidator.DecodeAndValidateURLValues(r, routeListFilter); err != nil { return nil, apierrors.LogAndReturn(logger, err, "Unable to decode request query parameters") } @@ -117,7 +112,7 @@ func (h *Route) create(r *http.Request) (*routing.Response, error) { logger := logr.FromContextOrDiscard(r.Context()).WithName("handlers.route.create") var payload payloads.RouteCreate - if err := h.decoderValidator.DecodeAndValidateJSONPayload(r, &payload); err != nil { + if err := h.requestValidator.DecodeAndValidateJSONPayload(r, &payload); err != nil { return nil, apierrors.LogAndReturn(logger, err, "failed to decode payload") } @@ -166,8 +161,8 @@ func (h *Route) insertDestinations(r *http.Request) (*routing.Response, error) { authInfo, _ := authorization.InfoFromContext(r.Context()) logger := logr.FromContextOrDiscard(r.Context()).WithName("handlers.route.insert-destinations") - var destinationCreatePayload payloads.DestinationListCreate - if err := h.decoderValidator.DecodeAndValidateJSONPayload(r, &destinationCreatePayload); err != nil { + var destinationCreatePayload payloads.RouteDestinationCreate + if err := h.requestValidator.DecodeAndValidateJSONPayload(r, &destinationCreatePayload); err != nil { return nil, apierrors.LogAndReturn(logger, err, "failed to decode payload") } @@ -236,23 +231,6 @@ func (h *Route) delete(r *http.Request) (*routing.Response, error) { return routing.NewResponse(http.StatusAccepted).WithHeader("Location", presenter.JobURLForRedirects(routeGUID, presenter.RouteDeleteOperation, h.serverURL)), nil } -func (h *Route) UnauthenticatedRoutes() []routing.Route { - return nil -} - -func (h *Route) AuthenticatedRoutes() []routing.Route { - return []routing.Route{ - {Method: "GET", Pattern: RoutePath, Handler: h.get}, - {Method: "GET", Pattern: RoutesPath, Handler: h.list}, - {Method: "GET", Pattern: RouteDestinationsPath, Handler: h.listDestinations}, - {Method: "POST", Pattern: RoutesPath, Handler: h.create}, - {Method: "DELETE", Pattern: RoutePath, Handler: h.delete}, - {Method: "POST", Pattern: RouteDestinationsPath, Handler: h.insertDestinations}, - {Method: "DELETE", Pattern: RouteDestinationPath, Handler: h.deleteDestination}, - {Method: "PATCH", Pattern: RoutePath, Handler: h.update}, - } -} - // Fetch Route and compose related Domain information within func (h *Route) lookupRouteAndDomain(ctx context.Context, logger logr.Logger, authInfo authorization.Info, routeGUID string) (repositories.RouteRecord, error) { route, err := h.routeRepo.GetRoute(ctx, authInfo, routeGUID) @@ -305,7 +283,7 @@ func (h *Route) update(r *http.Request) (*routing.Response, error) { } var payload payloads.RoutePatch - if err = h.decoderValidator.DecodeAndValidateJSONPayload(r, &payload); err != nil { + if err = h.requestValidator.DecodeAndValidateJSONPayload(r, &payload); err != nil { return nil, apierrors.LogAndReturn(logger, err, "failed to decode payload") } @@ -315,3 +293,20 @@ func (h *Route) update(r *http.Request) (*routing.Response, error) { } return routing.NewResponse(http.StatusOK).WithBody(presenter.ForRoute(route, h.serverURL)), nil } + +func (h *Route) UnauthenticatedRoutes() []routing.Route { + return nil +} + +func (h *Route) AuthenticatedRoutes() []routing.Route { + return []routing.Route{ + {Method: "GET", Pattern: RoutePath, Handler: h.get}, + {Method: "GET", Pattern: RoutesPath, Handler: h.list}, + {Method: "GET", Pattern: RouteDestinationsPath, Handler: h.listDestinations}, + {Method: "POST", Pattern: RoutesPath, Handler: h.create}, + {Method: "DELETE", Pattern: RoutePath, Handler: h.delete}, + {Method: "POST", Pattern: RouteDestinationsPath, Handler: h.insertDestinations}, + {Method: "DELETE", Pattern: RouteDestinationPath, Handler: h.deleteDestination}, + {Method: "PATCH", Pattern: RoutePath, Handler: h.update}, + } +} diff --git a/api/handlers/route_test.go b/api/handlers/route_test.go index ed205f803..593a53213 100644 --- a/api/handlers/route_test.go +++ b/api/handlers/route_test.go @@ -3,15 +3,17 @@ package handlers_test import ( "errors" "fmt" + "io" "net/http" - "regexp" "strings" apierrors "code.cloudfoundry.org/korifi/api/errors" . "code.cloudfoundry.org/korifi/api/handlers" "code.cloudfoundry.org/korifi/api/handlers/fake" + "code.cloudfoundry.org/korifi/api/payloads" "code.cloudfoundry.org/korifi/api/repositories" . "code.cloudfoundry.org/korifi/tests/matchers" + "code.cloudfoundry.org/korifi/tools" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -20,10 +22,11 @@ import ( var _ = Describe("Route", func() { var ( - routeRepo *fake.CFRouteRepository - domainRepo *fake.CFDomainRepository - appRepo *fake.CFAppRepository - spaceRepo *fake.SpaceRepository + routeRepo *fake.CFRouteRepository + domainRepo *fake.CFDomainRepository + appRepo *fake.CFAppRepository + spaceRepo *fake.SpaceRepository + requestValidator *fake.RequestValidator requestMethod string requestPath string @@ -66,8 +69,7 @@ var _ = Describe("Route", func() { Name: "test-space-guid", }, nil) - decoderValidator, err := NewDefaultDecoderValidator() - Expect(err).NotTo(HaveOccurred()) + requestValidator = new(fake.RequestValidator) apiHandler := NewRoute( *serverURL, @@ -75,7 +77,7 @@ var _ = Describe("Route", func() { domainRepo, appRepo, spaceRepo, - decoderValidator, + requestValidator, ) routerBuilder.LoadRoutes(apiHandler) }) @@ -166,11 +168,17 @@ var _ = Describe("Route", func() { }, nil) requestMethod = http.MethodGet - requestPath = "/v3/routes" + requestPath = "/v3/routes?foo=bar" requestBody = "" + + requestValidator.DecodeAndValidateURLValuesStub = decodeAndValidateURLValuesStub(&payloads.RouteList{}) }) It("returns the routes list", func() { + Expect(requestValidator.DecodeAndValidateURLValuesCallCount()).To(Equal(1)) + actualReq, _ := requestValidator.DecodeAndValidateURLValuesArgsForCall(0) + Expect(actualReq.URL.String()).To(HaveSuffix("/v3/routes?foo=bar")) + Expect(routeRepo.ListRoutesCallCount()).To(Equal(1)) _, actualAuthInfo, _ := routeRepo.ListRoutesArgsForCall(0) Expect(actualAuthInfo).To(Equal(authInfo)) @@ -184,6 +192,7 @@ var _ = Describe("Route", func() { Expect(rr).To(HaveHTTPHeaderWithValue("Content-Type", "application/json")) Expect(rr).To(HaveHTTPBody(SatisfyAll( MatchJSONPath("$.pagination.total_results", BeEquivalentTo(2)), + MatchJSONPath("$.pagination.first.href", "https://api.example.org/v3/routes?foo=bar"), MatchJSONPath("$.resources[0].guid", "test-route-guid"), MatchJSONPath("$.resources[0].url", "test-route-host.example.org/some_path"), MatchJSONPath("$.resources[1].guid", "other-test-route-guid"), @@ -193,34 +202,24 @@ var _ = Describe("Route", func() { When("query parameters are provided", func() { BeforeEach(func() { - requestPath += "?app_guids=my-app-guid&" + - "space_guids=my-space-guid&" + - "domain_guids=my-domain-guid&" + - "hosts=my-host&" + - "paths=/some/path" + payload := &payloads.RouteList{ + AppGUIDs: "a1,a2", + SpaceGUIDs: "s1,s2", + DomainGUIDs: "d1,d2", + Hosts: "h1,h2", + Paths: "p1,p2", + } + requestValidator.DecodeAndValidateURLValuesStub = decodeAndValidateURLValuesStub(payload) }) It("filters routes by that", func() { Expect(routeRepo.ListRoutesCallCount()).To(Equal(1)) _, _, message := routeRepo.ListRoutesArgsForCall(0) - Expect(message.AppGUIDs).To(ConsistOf("my-app-guid")) - Expect(message.SpaceGUIDs).To(ConsistOf("my-space-guid")) - Expect(message.DomainGUIDs).To(ConsistOf("my-domain-guid")) - Expect(message.Hosts).To(ConsistOf("my-host")) - Expect(message.Paths).To(ConsistOf("/some/path")) - - Expect(rr).To(HaveHTTPStatus(http.StatusOK)) - Expect(rr).To(HaveHTTPBody( - MatchJSONPath( - "$.pagination.first.href", - "https://api.example.org/v3/routes?"+ - "app_guids=my-app-guid&"+ - "space_guids=my-space-guid&"+ - "domain_guids=my-domain-guid&"+ - "hosts=my-host&"+ - "paths=/some/path", - ), - )) + Expect(message.AppGUIDs).To(ConsistOf("a1", "a2")) + Expect(message.SpaceGUIDs).To(ConsistOf("s1", "s2")) + Expect(message.DomainGUIDs).To(ConsistOf("d1", "d2")) + Expect(message.Hosts).To(ConsistOf("h1", "h2")) + Expect(message.Paths).To(ConsistOf("p1", "p2")) }) }) @@ -246,11 +245,11 @@ var _ = Describe("Route", func() { When("invalid query parameters are provided", func() { BeforeEach(func() { - requestPath += "?foo=my-app-guid" + requestValidator.DecodeAndValidateURLValuesReturns(errors.New("boom!")) }) - It("returns an Unknown key error", func() { - expectUnknownKeyError("The query parameter is invalid: Valid parameters are: .*") + It("returns an error", func() { + expectUnknownError() }) }) }) @@ -273,33 +272,34 @@ var _ = Describe("Route", func() { UpdatedAt: "update-time", }, nil) - requestBody = `{ - "host": "test-route-host", - "path": "/test-route-path", - "relationships": { - "domain": { - "data": { - "guid": "test-domain-guid" - } + requestBody = `doesn't matter` + + payload := payloads.RouteCreate{ + Host: "test-route-host", + Path: "/test-route-path", + Relationships: &payloads.RouteRelationships{ + Domain: payloads.Relationship{ + Data: &payloads.RelationshipData{GUID: "test-domain-guid"}, }, - "space": { - "data": { - "guid": "test-space-guid" - } - } - }, - "metadata": { - "labels": { - "label-key": "label-val" + Space: payloads.Relationship{ + Data: &payloads.RelationshipData{GUID: "test-space-guid"}, }, - "annotations": { - "annotation-key": "annotation-val" - } - } - }` + }, + Metadata: payloads.Metadata{ + Labels: map[string]string{"label-key": "label-val"}, + Annotations: map[string]string{"annotation-key": "annotation-val"}, + }, + } + requestValidator.DecodeAndValidateJSONPayloadStub = decodeAndValidateJSONPayloadStub(&payload) }) It("creates the route", func() { + Expect(requestValidator.DecodeAndValidateJSONPayloadCallCount()).To(Equal(1)) + actualReq, _ := requestValidator.DecodeAndValidateJSONPayloadArgsForCall(0) + bodyBytes, err := io.ReadAll(actualReq.Body) + Expect(err).NotTo(HaveOccurred()) + Expect(string(bodyBytes)).To(Equal(requestBody)) + Expect(spaceRepo.GetSpaceCallCount()).To(Equal(1)) _, actualAuthInfo, actualSpaceGUID := spaceRepo.GetSpaceArgsForCall(0) Expect(actualAuthInfo).To(Equal(authInfo)) @@ -330,78 +330,11 @@ var _ = Describe("Route", func() { When("the request body is invalid JSON", func() { BeforeEach(func() { - requestBody = `{` - }) - - It("returns a message parse erorr", func() { - expectBadRequestError() - }) - }) - - When("the request body includes an unknown description field", func() { - BeforeEach(func() { - requestBody = `{"description" : "Invalid Request"}` - }) - - It("returns an error", func() { - expectUnprocessableEntityError(`invalid request body: json: unknown field "description"`) - }) - }) - - When("the host is not a string", func() { - BeforeEach(func() { - requestBody = `{ - "host": 12345, - "relationships": { - "space": { - "data": { - "guid": "2f35885d-0c9d-4423-83ad-fd05066f8576" - } - } - } - }` - }) - - It("returns an error", func() { - expectUnprocessableEntityError("Host must be a string") - }) - }) - - When("the request body is missing the domain relationship", func() { - BeforeEach(func() { - requestBody = `{ - "host": "test-route-host", - "relationships": { - "space": { - "data": { - "guid": "0c78dd5d-c723-4f2e-b168-df3c3e1d0806" - } - } - } - }` + requestValidator.DecodeAndValidateJSONPayloadReturns(errors.New("boom")) }) It("returns an error", func() { - expectUnprocessableEntityError("Data is a required field") - }) - }) - - When("the request body is missing the space relationship", func() { - BeforeEach(func() { - requestBody = `{ - "host": "test-route-host", - "relationships": { - "domain": { - "data": { - "guid": "0b78dd5d-c723-4f2e-b168-df3c3e1d0806" - } - } - } - }` - }) - - It("returns an error", func() { - expectUnprocessableEntityError("Data is a required field") + expectUnknownError() }) }) @@ -490,29 +423,30 @@ var _ = Describe("Route", func() { }, }, nil) - requestBody = `{ - "metadata": { - "labels": { - "env": "production", - "foo.example.com/my-identifier": "aruba" - }, - "annotations": { - "hello": "there", - "foo.example.com/lorem-ipsum": "Lorem ipsum." - } - } - }` + requestBody = `doesn't matter` + + payload := payloads.RoutePatch{ + Metadata: payloads.MetadataPatch{ + Annotations: map[string]*string{"a": tools.PtrTo("av")}, + Labels: map[string]*string{"l": tools.PtrTo("lv")}, + }, + } + requestValidator.DecodeAndValidateJSONPayloadStub = decodeAndValidateJSONPayloadStub(&payload) }) It("patches the route", func() { + Expect(requestValidator.DecodeAndValidateJSONPayloadCallCount()).To(Equal(1)) + actualReq, _ := requestValidator.DecodeAndValidateJSONPayloadArgsForCall(0) + bodyBytes, err := io.ReadAll(actualReq.Body) + Expect(err).NotTo(HaveOccurred()) + Expect(string(bodyBytes)).To(Equal(requestBody)) + Expect(routeRepo.PatchRouteMetadataCallCount()).To(Equal(1)) _, _, msg := routeRepo.PatchRouteMetadataArgsForCall(0) Expect(msg.RouteGUID).To(Equal("test-route-guid")) Expect(msg.SpaceGUID).To(Equal(spaceGUID)) - Expect(msg.Annotations).To(HaveKeyWithValue("hello", PointTo(Equal("there")))) - Expect(msg.Annotations).To(HaveKeyWithValue("foo.example.com/lorem-ipsum", PointTo(Equal("Lorem ipsum.")))) - Expect(msg.Labels).To(HaveKeyWithValue("env", PointTo(Equal("production")))) - Expect(msg.Labels).To(HaveKeyWithValue("foo.example.com/my-identifier", PointTo(Equal("aruba")))) + Expect(msg.Annotations).To(HaveKeyWithValue("a", PointTo(Equal("av")))) + Expect(msg.Labels).To(HaveKeyWithValue("l", PointTo(Equal("lv")))) Expect(rr).To(HaveHTTPStatus(http.StatusOK)) Expect(rr).To(HaveHTTPHeaderWithValue("Content-Type", "application/json")) @@ -554,71 +488,13 @@ var _ = Describe("Route", func() { }) }) - When("a label is invalid", func() { - When("the prefix is cloudfoundry.org", func() { - BeforeEach(func() { - requestBody = `{ - "metadata": { - "labels": { - "cloudfoundry.org/test": "production" - } - } - }` - }) - - It("returns an unprocessable entity error", func() { - expectUnprocessableEntityError(`Labels and annotations cannot begin with "cloudfoundry.org" or its subdomains`) - }) - }) - - When("the prefix is a subdomain of cloudfoundry.org", func() { - BeforeEach(func() { - requestBody = `{ - "metadata": { - "labels": { - "korifi.cloudfoundry.org/test": "production" - } - } - }` - }) - - It("returns an unprocessable entity error", func() { - expectUnprocessableEntityError(`Labels and annotations cannot begin with "cloudfoundry.org" or its subdomains`) - }) - }) - }) - - When("an annotation is invalid", func() { - When("the prefix is cloudfoundry.org", func() { - BeforeEach(func() { - requestBody = `{ - "metadata": { - "annotations": { - "cloudfoundry.org/test": "there" - } - } - }` - }) - - It("returns an unprocessable entity error", func() { - expectUnprocessableEntityError(`Labels and annotations cannot begin with "cloudfoundry.org" or its subdomains`) - }) - - When("the prefix is a subdomain of cloudfoundry.org", func() { - BeforeEach(func() { - requestBody = `{ - "metadata": { - "annotations": { - "korifi.cloudfoundry.org/test": "there" - } - } - }` - }) - - It("returns an unprocessable entity error", func() { - expectUnprocessableEntityError(`Labels and annotations cannot begin with "cloudfoundry.org" or its subdomains`) - }) - }) + When("the request is invalid", func() { + BeforeEach(func() { + requestValidator.DecodeAndValidateJSONPayloadReturns(errors.New("boom")) + }) + + It("returns an error", func() { + expectUnknownError() }) }) }) @@ -693,28 +569,37 @@ var _ = Describe("Route", func() { requestMethod = http.MethodPost requestPath = "/v3/routes/test-route-guid/destinations" - requestBody = `{ - "destinations": [ + requestBody = `doesn't matter` + + payload := payloads.RouteDestinationCreate{ + Destinations: []payloads.RouteDestination{ { - "app": { - "guid": "app-1-guid" - } + App: payloads.AppResource{ + GUID: "app-1-guid", + }, }, { - "app": { - "guid": "app-2-guid", - "process": { - "type": "queue" - } + App: payloads.AppResource{ + GUID: "app-2-guid", + Process: &payloads.DestinationAppProcess{ + Type: "queue", + }, }, - "port": 1234, - "protocol": "http1" - } - ] - }` + Port: tools.PtrTo(1234), + Protocol: tools.PtrTo("http1"), + }, + }, + } + requestValidator.DecodeAndValidateJSONPayloadStub = decodeAndValidateJSONPayloadStub(&payload) }) It("adds the destinations to the route", func() { + Expect(requestValidator.DecodeAndValidateJSONPayloadCallCount()).To(Equal(1)) + actualReq, _ := requestValidator.DecodeAndValidateJSONPayloadArgsForCall(0) + bodyBytes, err := io.ReadAll(actualReq.Body) + Expect(err).NotTo(HaveOccurred()) + Expect(string(bodyBytes)).To(Equal(requestBody)) + Expect(routeRepo.GetRouteCallCount()).To(Equal(1)) _, actualAuthInfo, actualRouteGUID := routeRepo.GetRouteArgsForCall(0) Expect(actualAuthInfo).To(Equal(authInfo)) @@ -797,86 +682,13 @@ var _ = Describe("Route", func() { }) }) - When("JSON is invalid", func() { + When("request is invalid", func() { BeforeEach(func() { - requestBody = `{` + requestValidator.DecodeAndValidateJSONPayloadReturns(errors.New("boom")) }) It("returns an error", func() { - expectBadRequestError() - }) - }) - - When("app is missing", func() { - BeforeEach(func() { - requestBody = `{ - "destinations": [ - { "port": 9000, "protocol": "http1" } - ] - }` - }) - - It("returns an error and doesn't add the destinations", func() { - Expect(routeRepo.AddDestinationsToRouteCallCount()).To(Equal(0)) - expectUnprocessableEntityError("App is a required field") - }) - }) - - When("app GUID is missing", func() { - BeforeEach(func() { - requestBody = `{ - "destinations": [ - { "app": {}, "port": 9000, "protocol": "http1" } - ] - }` - }) - - It("returns an error and doesn't add the destinations", func() { - Expect(routeRepo.AddDestinationsToRouteCallCount()).To(Equal(0)) - expectUnprocessableEntityError("GUID is a required field") - }) - }) - - When("process type is missing", func() { - BeforeEach(func() { - requestBody = `{ - "destinations": [ - { - "app": { - "guid": "01856e12-8ee8-11e9-98a5-bb397dbc818f", - "process": {} - }, - "port": 9000, - "protocol": "http1" - } - ] - }` - }) - - It("returns an error and doesn't add the destinations", func() { - Expect(routeRepo.AddDestinationsToRouteCallCount()).To(Equal(0)) - expectUnprocessableEntityError("Type is a required field") - }) - }) - - When("destination protocol is not http1", func() { - BeforeEach(func() { - requestBody = `{ - "destinations": [ - { - "app": { - "guid": "01856e12-8ee8-11e9-98a5-bb397dbc818f" - }, - "port": 9000, - "protocol": "http" - } - ] - }` - }) - - It("returns an error and doesn't add the destinations", func() { - expectUnprocessableEntityError(regexp.QuoteMeta("Protocol must be one of [http1]")) - Expect(routeRepo.AddDestinationsToRouteCallCount()).To(Equal(0)) + expectUnknownError() }) }) }) diff --git a/api/payloads/destination.go b/api/payloads/destination.go deleted file mode 100644 index f6e96744f..000000000 --- a/api/payloads/destination.go +++ /dev/null @@ -1,58 +0,0 @@ -package payloads - -import ( - "code.cloudfoundry.org/korifi/api/repositories" - korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" -) - -type DestinationListCreate struct { - Destinations []Destination `json:"destinations" validate:"required,dive"` -} - -type Destination struct { - App *AppResource `json:"app" validate:"required"` - Port *int `json:"port"` - Protocol *string `json:"protocol" validate:"omitempty,oneof=http1"` -} - -type AppResource struct { - GUID string `json:"guid" validate:"required"` - Process *DestinationAppProcess `json:"process"` -} - -type DestinationAppProcess struct { - Type string `json:"type" validate:"required"` -} - -func (dc DestinationListCreate) ToMessage(routeRecord repositories.RouteRecord) repositories.AddDestinationsToRouteMessage { - addDestinations := make([]repositories.DestinationMessage, 0, len(dc.Destinations)) - for _, destination := range dc.Destinations { - processType := korifiv1alpha1.ProcessTypeWeb - if destination.App.Process != nil { - processType = destination.App.Process.Type - } - - port := 8080 - if destination.Port != nil { - port = *destination.Port - } - - protocol := "http1" - if destination.Protocol != nil { - protocol = *destination.Protocol - } - - addDestinations = append(addDestinations, repositories.DestinationMessage{ - AppGUID: destination.App.GUID, - ProcessType: processType, - Port: port, - Protocol: protocol, - }) - } - return repositories.AddDestinationsToRouteMessage{ - RouteGUID: routeRecord.GUID, - SpaceGUID: routeRecord.SpaceGUID, - ExistingDestinations: routeRecord.Destinations, - NewDestinations: addDestinations, - } -} diff --git a/api/payloads/route.go b/api/payloads/route.go index 827554aeb..c2fb2134a 100644 --- a/api/payloads/route.go +++ b/api/payloads/route.go @@ -4,19 +4,25 @@ import ( "net/url" "code.cloudfoundry.org/korifi/api/payloads/parse" + "code.cloudfoundry.org/korifi/api/payloads/validation" "code.cloudfoundry.org/korifi/api/repositories" + korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" + jellidation "github.com/jellydator/validation" ) type RouteCreate struct { - Host string `json:"host" validate:"required"` // TODO: Not required when we support private domains - Path string `json:"path"` - Relationships RouteRelationships `json:"relationships" validate:"required"` - Metadata Metadata `json:"metadata"` + Host string `json:"host"` + Path string `json:"path"` + Relationships *RouteRelationships `json:"relationships"` + Metadata Metadata `json:"metadata"` } -type RouteRelationships struct { - Domain Relationship `json:"domain" validate:"required"` - Space Relationship `json:"space" validate:"required"` +func (p RouteCreate) Validate() error { + return jellidation.ValidateStruct(&p, + jellidation.Field(&p.Host, jellidation.Required), + jellidation.Field(&p.Relationships, jellidation.NotNil), + jellidation.Field(&p.Metadata), + ) } func (p RouteCreate) ToMessage(domainNamespace, domainName string) repositories.CreateRouteMessage { @@ -32,6 +38,18 @@ func (p RouteCreate) ToMessage(domainNamespace, domainName string) repositories. } } +type RouteRelationships struct { + Domain Relationship `json:"domain" validate:"required"` + Space Relationship `json:"space" validate:"required"` +} + +func (r RouteRelationships) Validate() error { + return jellidation.ValidateStruct(&r, + jellidation.Field(&r.Domain, validation.StrictlyRequired), + jellidation.Field(&r.Space, validation.StrictlyRequired), + ) +} + type RouteList struct { AppGUIDs string SpaceGUIDs string @@ -40,7 +58,7 @@ type RouteList struct { Paths string } -func (p *RouteList) ToMessage() repositories.ListRoutesMessage { +func (p RouteList) ToMessage() repositories.ListRoutesMessage { return repositories.ListRoutesMessage{ AppGUIDs: parse.ArrayParam(p.AppGUIDs), SpaceGUIDs: parse.ArrayParam(p.SpaceGUIDs), @@ -50,7 +68,7 @@ func (p *RouteList) ToMessage() repositories.ListRoutesMessage { } } -func (p *RouteList) SupportedKeys() []string { +func (p RouteList) SupportedKeys() []string { return []string{"app_guids", "space_guids", "domain_guids", "hosts", "paths", "per_page", "page"} } @@ -67,13 +85,97 @@ type RoutePatch struct { Metadata MetadataPatch `json:"metadata"` } -func (a *RoutePatch) ToMessage(routeGUID, spaceGUID string) repositories.PatchRouteMetadataMessage { +func (p RoutePatch) Validate() error { + return jellidation.ValidateStruct(&p, + jellidation.Field(&p.Metadata), + ) +} + +func (p RoutePatch) ToMessage(routeGUID, spaceGUID string) repositories.PatchRouteMetadataMessage { return repositories.PatchRouteMetadataMessage{ RouteGUID: routeGUID, SpaceGUID: spaceGUID, MetadataPatch: repositories.MetadataPatch{ - Annotations: a.Metadata.Annotations, - Labels: a.Metadata.Labels, + Annotations: p.Metadata.Annotations, + Labels: p.Metadata.Labels, }, } } + +type RouteDestinationCreate struct { + Destinations []RouteDestination `json:"destinations"` +} + +func (r RouteDestinationCreate) Validate() error { + return jellidation.ValidateStruct(&r, + jellidation.Field(&r.Destinations), + ) +} + +type RouteDestination struct { + App AppResource `json:"app"` + Port *int `json:"port"` + Protocol *string `json:"protocol"` +} + +func (r RouteDestination) Validate() error { + return jellidation.ValidateStruct(&r, + jellidation.Field(&r.App), + jellidation.Field(&r.Protocol, validation.OneOf("http1")), + ) +} + +type AppResource struct { + GUID string `json:"guid"` + Process *DestinationAppProcess `json:"process"` +} + +func (a AppResource) Validate() error { + return jellidation.ValidateStruct(&a, + jellidation.Field(&a.GUID, jellidation.Required), + jellidation.Field(&a.Process), + ) +} + +type DestinationAppProcess struct { + Type string `json:"type"` +} + +func (p DestinationAppProcess) Validate() error { + return jellidation.ValidateStruct(&p, + jellidation.Field(&p.Type, jellidation.Required), + ) +} + +func (dc RouteDestinationCreate) ToMessage(routeRecord repositories.RouteRecord) repositories.AddDestinationsToRouteMessage { + addDestinations := make([]repositories.DestinationMessage, 0, len(dc.Destinations)) + for _, destination := range dc.Destinations { + processType := korifiv1alpha1.ProcessTypeWeb + if destination.App.Process != nil { + processType = destination.App.Process.Type + } + + port := 8080 + if destination.Port != nil { + port = *destination.Port + } + + protocol := "http1" + if destination.Protocol != nil { + protocol = *destination.Protocol + } + + addDestinations = append(addDestinations, repositories.DestinationMessage{ + AppGUID: destination.App.GUID, + ProcessType: processType, + Port: port, + Protocol: protocol, + }) + } + return repositories.AddDestinationsToRouteMessage{ + RouteGUID: routeRecord.GUID, + SpaceGUID: routeRecord.SpaceGUID, + ExistingDestinations: routeRecord.Destinations, + NewDestinations: addDestinations, + } +} diff --git a/api/payloads/route_test.go b/api/payloads/route_test.go index 7dad4f7c5..cb76d9001 100644 --- a/api/payloads/route_test.go +++ b/api/payloads/route_test.go @@ -3,20 +3,35 @@ package payloads_test import ( "net/http" + "code.cloudfoundry.org/korifi/api/errors" "code.cloudfoundry.org/korifi/api/payloads" + "code.cloudfoundry.org/korifi/tools" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/onsi/gomega/gstruct" ) var _ = Describe("RouteList", func() { Describe("decode from url values", func() { - It("succeeds", func() { - routeList := payloads.RouteList{} - req, err := http.NewRequest("GET", "http://foo.com/bar?app_guids=app_guid&space_guids=space_guid&domain_guids=domain_guid&hosts=host&paths=path", nil) - Expect(err).NotTo(HaveOccurred()) - err = validator.DecodeAndValidateURLValues(req, &routeList) + var ( + routeList payloads.RouteList + decodeErr error + params string + ) + BeforeEach(func() { + routeList = payloads.RouteList{} + params = "app_guids=app_guid&space_guids=space_guid&domain_guids=domain_guid&hosts=host&paths=path" + }) + + JustBeforeEach(func() { + req, err := http.NewRequest("GET", "http://foo.com/bar?"+params, nil) Expect(err).NotTo(HaveOccurred()) + decodeErr = validator.DecodeAndValidateURLValues(req, &routeList) + }) + + It("succeeds", func() { + Expect(decodeErr).NotTo(HaveOccurred()) Expect(routeList).To(Equal(payloads.RouteList{ AppGUIDs: "app_guid", SpaceGUIDs: "space_guid", @@ -25,5 +40,228 @@ var _ = Describe("RouteList", func() { Paths: "path", })) }) + + When("it contains an invalid key", func() { + BeforeEach(func() { + params = "foo=bar" + }) + + It("fails", func() { + Expect(decodeErr).To(MatchError("unsupported query parameter: foo")) + }) + }) + }) +}) + +var _ = Describe("RouteCreate", func() { + var ( + createPayload payloads.RouteCreate + routeCreate *payloads.RouteCreate + validatorErr error + apiError errors.ApiError + ) + + BeforeEach(func() { + routeCreate = new(payloads.RouteCreate) + createPayload = payloads.RouteCreate{ + Host: "h1", + Path: "p1", + Relationships: &payloads.RouteRelationships{ + Domain: payloads.Relationship{ + Data: &payloads.RelationshipData{ + GUID: "d1", + }, + }, + Space: payloads.Relationship{ + Data: &payloads.RelationshipData{ + GUID: "s1", + }, + }, + }, + Metadata: payloads.Metadata{ + Annotations: map[string]string{"a": "av"}, + Labels: map[string]string{"l": "lv"}, + }, + } + }) + + JustBeforeEach(func() { + validatorErr = validator.DecodeAndValidateJSONPayload(createRequest(createPayload), routeCreate) + apiError, _ = validatorErr.(errors.ApiError) + }) + + It("succeeds", func() { + Expect(validatorErr).NotTo(HaveOccurred()) + Expect(routeCreate).To(gstruct.PointTo(Equal(createPayload))) + }) + + When("host is blank", func() { + BeforeEach(func() { + createPayload.Host = "" + }) + + It("fails", func() { + Expect(apiError).To(HaveOccurred()) + Expect(apiError.Detail()).To(ContainSubstring("host cannot be blank")) + }) + }) + + When("relationships is empty", func() { + BeforeEach(func() { + createPayload.Relationships = nil + }) + + It("fails", func() { + Expect(apiError).To(HaveOccurred()) + Expect(apiError.Detail()).To(ContainSubstring("relationships is required")) + }) + }) + + When("domain guid is blank", func() { + BeforeEach(func() { + createPayload.Relationships.Domain.Data.GUID = "" + }) + + It("fails", func() { + Expect(apiError).To(HaveOccurred()) + Expect(apiError.Detail()).To(ContainSubstring("relationships.domain.data.guid cannot be blank")) + }) + }) + + When("space guid is blank", func() { + BeforeEach(func() { + createPayload.Relationships.Space.Data.GUID = "" + }) + + It("fails", func() { + Expect(apiError).To(HaveOccurred()) + Expect(apiError.Detail()).To(ContainSubstring("relationships.space.data.guid cannot be blank")) + }) + }) + + When("metadata uses the cloudfoundry domain", func() { + BeforeEach(func() { + createPayload.Metadata.Labels["foo.cloudfoundry.org/bar"] = "baz" + }) + + It("fails", func() { + Expect(apiError).To(HaveOccurred()) + Expect(apiError.Detail()).To(ContainSubstring("cannot use the cloudfoundry.org domain")) + }) + }) +}) + +var _ = Describe("RoutePatch", func() { + var ( + patchPayload payloads.RoutePatch + routePatch *payloads.RoutePatch + validatorErr error + apiError errors.ApiError + ) + + BeforeEach(func() { + routePatch = new(payloads.RoutePatch) + patchPayload = payloads.RoutePatch{ + Metadata: payloads.MetadataPatch{ + Annotations: map[string]*string{"a": tools.PtrTo("av")}, + Labels: map[string]*string{"l": tools.PtrTo("lv")}, + }, + } + }) + + JustBeforeEach(func() { + validatorErr = validator.DecodeAndValidateJSONPayload(createRequest(patchPayload), routePatch) + apiError, _ = validatorErr.(errors.ApiError) + }) + + It("succeeds", func() { + Expect(validatorErr).NotTo(HaveOccurred()) + Expect(routePatch).To(gstruct.PointTo(Equal(patchPayload))) + }) + + When("metadata uses the cloudfoundry domain", func() { + BeforeEach(func() { + patchPayload.Metadata.Labels["foo.cloudfoundry.org/bar"] = tools.PtrTo("baz") + }) + + It("fails", func() { + Expect(apiError).To(HaveOccurred()) + Expect(apiError.Detail()).To(ContainSubstring("cannot use the cloudfoundry.org domain")) + }) + }) +}) + +var _ = Describe("Add destination", func() { + var ( + addPayload payloads.RouteDestinationCreate + destinationAdd *payloads.RouteDestinationCreate + validatorErr error + apiError errors.ApiError + ) + + BeforeEach(func() { + destinationAdd = new(payloads.RouteDestinationCreate) + addPayload = payloads.RouteDestinationCreate{ + Destinations: []payloads.RouteDestination{ + { + App: payloads.AppResource{ + GUID: "app-1-guid", + }, + }, + { + App: payloads.AppResource{ + GUID: "app-2-guid", + Process: &payloads.DestinationAppProcess{ + Type: "queue", + }, + }, + Port: tools.PtrTo(1234), + Protocol: tools.PtrTo("http1"), + }, + }, + } + }) + + JustBeforeEach(func() { + validatorErr = validator.DecodeAndValidateJSONPayload(createRequest(addPayload), destinationAdd) + apiError, _ = validatorErr.(errors.ApiError) + }) + + It("succeeds", func() { + Expect(validatorErr).NotTo(HaveOccurred()) + Expect(destinationAdd).To(gstruct.PointTo(Equal(addPayload))) + }) + + When("app guid is empty", func() { + BeforeEach(func() { + addPayload.Destinations[0].App.GUID = "" + }) + + It("fails", func() { + Expect(apiError).To(HaveOccurred()) + Expect(apiError.Detail()).To(ContainSubstring("guid cannot be blank")) + }) + }) + + When("process type is empty", func() { + BeforeEach(func() { + addPayload.Destinations[1].App.Process.Type = "" + }) + + It("fails", func() { + Expect(apiError).To(HaveOccurred()) + Expect(apiError.Detail()).To(ContainSubstring("type cannot be blank")) + }) + }) + + When("protocol is not http1", func() { + BeforeEach(func() { + addPayload.Destinations[1].Protocol = tools.PtrTo("http") + }) + + It("fails", func() { + Expect(apiError).To(HaveOccurred()) + Expect(apiError.Detail()).To(ContainSubstring("value must be one of: http1")) + }) }) }) From d266e1a46498e81934e75612a8326d76062b6a7a Mon Sep 17 00:00:00 2001 From: Kieron Browne Date: Mon, 26 Jun 2023 16:45:35 +0100 Subject: [PATCH 4/5] Move service binding payload validation to payloads And migrate from go-playground to jellydator validation. --- api/handlers/service_binding.go | 17 +- api/handlers/service_binding_test.go | 284 +++++++-------------------- api/payloads/domain_test.go | 2 +- api/payloads/relationship.go | 9 +- api/payloads/relationship_test.go | 2 +- api/payloads/service_binding.go | 32 ++- api/payloads/service_binding_test.go | 147 ++++++++++++++ 7 files changed, 264 insertions(+), 229 deletions(-) diff --git a/api/handlers/service_binding.go b/api/handlers/service_binding.go index 62e6a5c56..6383aab37 100644 --- a/api/handlers/service_binding.go +++ b/api/handlers/service_binding.go @@ -26,7 +26,7 @@ type ServiceBinding struct { serviceBindingRepo CFServiceBindingRepository serviceInstanceRepo CFServiceInstanceRepository serverURL url.URL - decoderValidator *DecoderValidator + requestValidator RequestValidator } //counterfeiter:generate -o fake -fake-name CFServiceBindingRepository . CFServiceBindingRepository @@ -38,13 +38,13 @@ type CFServiceBindingRepository interface { UpdateServiceBinding(context.Context, authorization.Info, repositories.UpdateServiceBindingMessage) (repositories.ServiceBindingRecord, error) } -func NewServiceBinding(serverURL url.URL, serviceBindingRepo CFServiceBindingRepository, appRepo CFAppRepository, serviceInstanceRepo CFServiceInstanceRepository, decoderValidator *DecoderValidator) *ServiceBinding { +func NewServiceBinding(serverURL url.URL, serviceBindingRepo CFServiceBindingRepository, appRepo CFAppRepository, serviceInstanceRepo CFServiceInstanceRepository, requestValidator RequestValidator) *ServiceBinding { return &ServiceBinding{ appRepo: appRepo, serviceInstanceRepo: serviceInstanceRepo, serviceBindingRepo: serviceBindingRepo, serverURL: serverURL, - decoderValidator: decoderValidator, + requestValidator: requestValidator, } } @@ -53,7 +53,7 @@ func (h *ServiceBinding) create(r *http.Request) (*routing.Response, error) { logger := logr.FromContextOrDiscard(r.Context()).WithName("handlers.service-binding.create") var payload payloads.ServiceBindingCreate - if err := h.decoderValidator.DecodeAndValidateJSONPayload(r, &payload); err != nil { + if err := h.requestValidator.DecodeAndValidateJSONPayload(r, &payload); err != nil { return nil, apierrors.LogAndReturn(logger, err, "failed to decode payload") } @@ -102,13 +102,8 @@ func (h *ServiceBinding) list(r *http.Request) (*routing.Response, error) { authInfo, _ := authorization.InfoFromContext(r.Context()) logger := logr.FromContextOrDiscard(r.Context()).WithName("handlers.service-binding.list") - if err := r.ParseForm(); err != nil { - return nil, apierrors.LogAndReturn(logger, apierrors.NewUnprocessableEntityError(err, "unable to parse query"), "Unable to parse request query parameters") - } - listFilter := new(payloads.ServiceBindingList) - err := payloads.Decode(listFilter, r.Form) - if err != nil { + if err := h.requestValidator.DecodeAndValidateURLValues(r, listFilter); err != nil { return nil, apierrors.LogAndReturn(logger, err, "Unable to decode request query parameters") } @@ -141,7 +136,7 @@ func (h *ServiceBinding) update(r *http.Request) (*routing.Response, error) { // serviceBindingGUID := routing.URLParam(r, "guid") var payload payloads.ServiceBindingUpdate - if err := h.decoderValidator.DecodeAndValidateJSONPayload(r, &payload); err != nil { + if err := h.requestValidator.DecodeAndValidateJSONPayload(r, &payload); err != nil { return nil, apierrors.LogAndReturn(logger, err, "failed to decode payload") } diff --git a/api/handlers/service_binding_test.go b/api/handlers/service_binding_test.go index 71772f527..548216170 100644 --- a/api/handlers/service_binding_test.go +++ b/api/handlers/service_binding_test.go @@ -2,13 +2,14 @@ package handlers_test import ( "errors" + "io" "net/http" - "regexp" "strings" apierrors "code.cloudfoundry.org/korifi/api/errors" . "code.cloudfoundry.org/korifi/api/handlers" "code.cloudfoundry.org/korifi/api/handlers/fake" + "code.cloudfoundry.org/korifi/api/payloads" "code.cloudfoundry.org/korifi/api/repositories" . "code.cloudfoundry.org/korifi/tests/matchers" "code.cloudfoundry.org/korifi/tools" @@ -26,6 +27,7 @@ var _ = Describe("ServiceBinding", func() { serviceBindingRepo *fake.CFServiceBindingRepository appRepo *fake.CFAppRepository serviceInstanceRepo *fake.CFServiceInstanceRepository + requestValidator *fake.RequestValidator ) BeforeEach(func() { @@ -46,15 +48,14 @@ var _ = Describe("ServiceBinding", func() { SpaceGUID: "space-guid", }, nil) - decoderValidator, err := NewDefaultDecoderValidator() - Expect(err).NotTo(HaveOccurred()) + requestValidator = new(fake.RequestValidator) apiHandler := NewServiceBinding( *serverURL, serviceBindingRepo, appRepo, serviceInstanceRepo, - decoderValidator, + requestValidator, ) routerBuilder.LoadRoutes(apiHandler) }) @@ -67,31 +68,42 @@ var _ = Describe("ServiceBinding", func() { }) Describe("POST /v3/service_credential_bindings", func() { + var payload payloads.ServiceBindingCreate + BeforeEach(func() { requestMethod = http.MethodPost requestPath = "/v3/service_credential_bindings" - requestBody = `{ - "type": "app", - "relationships": { - "app": { - "data": { - "guid": "app-guid" - } - }, - "service_instance": { - "data": { - "guid": "service-instance-guid" - } - } - } - }` + requestBody = "doesn't matter" serviceBindingRepo.CreateServiceBindingReturns(repositories.ServiceBindingRecord{ GUID: "service-binding-guid", }, nil) + + payload = payloads.ServiceBindingCreate{ + Relationships: &payloads.ServiceBindingRelationships{ + App: &payloads.Relationship{ + Data: &payloads.RelationshipData{ + GUID: "app-guid", + }, + }, + ServiceInstance: &payloads.Relationship{ + Data: &payloads.RelationshipData{ + GUID: "service-instance-guid", + }, + }, + }, + Type: "app", + } + requestValidator.DecodeAndValidateJSONPayloadStub = decodeAndValidateJSONPayloadStub(&payload) }) It("creates a service binding", func() { + Expect(requestValidator.DecodeAndValidateJSONPayloadCallCount()).To(Equal(1)) + actualReq, _ := requestValidator.DecodeAndValidateJSONPayloadArgsForCall(0) + bodyBytes, err := io.ReadAll(actualReq.Body) + Expect(err).NotTo(HaveOccurred()) + Expect(string(bodyBytes)).To(Equal(requestBody)) + Expect(appRepo.GetAppCallCount()).To(Equal(1)) _, actualAuthInfo, actualAppGUID := appRepo.GetAppArgsForCall(0) Expect(actualAuthInfo).To(Equal(authInfo)) @@ -119,88 +131,11 @@ var _ = Describe("ServiceBinding", func() { When("the request body is invalid json", func() { BeforeEach(func() { - requestBody = "{" + requestValidator.DecodeAndValidateJSONPayloadReturns(errors.New("boom")) }) - It("returns an error and doesn't create the service binding", func() { - expectBadRequestError() - Expect(serviceBindingRepo.CreateServiceBindingCallCount()).To(Equal(0)) - }) - }) - - When(`the type is "key"`, func() { - BeforeEach(func() { - requestBody = `{ - "type": "key", - "relationships": { - "app": { - "data": { - "guid": "app-guid" - } - }, - "service_instance": { - "data": { - "guid": "service-instance-guid" - } - } - } - }` - }) - - It("returns an error and doesn't create the ServiceBinding", func() { - expectUnprocessableEntityError(regexp.QuoteMeta("Type must be one of [app]")) - Expect(serviceBindingRepo.CreateServiceBindingCallCount()).To(Equal(0)) - }) - }) - - When("all relationships are missing", func() { - BeforeEach(func() { - requestBody = `{ "type": "app" }` - }) - - It("returns an error and doesn't create the ServiceBinding", func() { - expectUnprocessableEntityError(`Relationships is a required field`) - Expect(serviceBindingRepo.CreateServiceBindingCallCount()).To(Equal(0)) - }) - }) - - When("the app relationship is missing", func() { - BeforeEach(func() { - requestBody = `{ - "type": "app", - "relationships": { - "service_instance": { - "data": { - "guid": "service-instance-guid" - } - } - } - }` - }) - - It("returns an error and doesn't create the ServiceBinding", func() { - expectUnprocessableEntityError(`App is a required field`) - Expect(serviceBindingRepo.CreateServiceBindingCallCount()).To(Equal(0)) - }) - }) - - When("the service_instance relationship is missing", func() { - BeforeEach(func() { - requestBody = `{ - "type": "app", - "relationships": { - "app": { - "data": { - "guid": "app-guid" - } - } - } - }` - }) - - It("returns an error and doesn't create the ServiceBinding", func() { - expectUnprocessableEntityError("ServiceInstance is a required field") - Expect(serviceBindingRepo.CreateServiceBindingCallCount()).To(Equal(0)) + It("returns an error", func() { + expectUnknownError() }) }) @@ -290,20 +225,35 @@ var _ = Describe("ServiceBinding", func() { BeforeEach(func() { requestMethod = http.MethodGet requestBody = "" - requestPath = "/v3/service_credential_bindings" + requestPath = "/v3/service_credential_bindings?foo=bar" serviceBindingRepo.ListServiceBindingsReturns([]repositories.ServiceBindingRecord{ {GUID: "service-binding-guid", AppGUID: "app-guid"}, }, nil) appRepo.ListAppsReturns([]repositories.AppRecord{{Name: "some-app-name"}}, nil) + + payload := payloads.ServiceBindingList{ + AppGUIDs: "a1,a2", + ServiceInstanceGUIDs: "s1,s2", + } + requestValidator.DecodeAndValidateURLValuesStub = decodeAndValidateURLValuesStub(&payload) }) It("returns the list of ServiceBindings", func() { + Expect(requestValidator.DecodeAndValidateURLValuesCallCount()).To(Equal(1)) + actualReq, _ := requestValidator.DecodeAndValidateURLValuesArgsForCall(0) + Expect(actualReq.URL.String()).To(HaveSuffix(requestPath)) + + Expect(serviceBindingRepo.ListServiceBindingsCallCount()).To(Equal(1)) + _, _, message := serviceBindingRepo.ListServiceBindingsArgsForCall(0) + Expect(message.AppGUIDs).To(ConsistOf([]string{"a1", "a2"})) + Expect(message.ServiceInstanceGUIDs).To(ConsistOf([]string{"s1", "s2"})) + Expect(rr).To(HaveHTTPStatus(http.StatusOK)) Expect(rr).To(HaveHTTPHeaderWithValue("Content-Type", "application/json")) Expect(rr).To(HaveHTTPBody(SatisfyAll( MatchJSONPath("$.pagination.total_results", BeEquivalentTo(1)), - MatchJSONPath("$.pagination.first.href", "https://api.example.org/v3/service_credential_bindings"), + MatchJSONPath("$.pagination.first.href", "https://api.example.org/v3/service_credential_bindings?foo=bar"), MatchJSONPath("$.resources[0].guid", "service-binding-guid"), ))) }) @@ -320,7 +270,10 @@ var _ = Describe("ServiceBinding", func() { When("an include=app query parameter is specified", func() { BeforeEach(func() { - requestPath += "?include=app" + payload := payloads.ServiceBindingList{ + Include: "app", + } + requestValidator.DecodeAndValidateURLValuesStub = decodeAndValidateURLValuesStub(&payload) }) It("includes app data in the response", func() { @@ -332,47 +285,13 @@ var _ = Describe("ServiceBinding", func() { }) }) - When("an app_guids query parameter is provided", func() { - BeforeEach(func() { - requestPath += "?app_guids=1,2,3" - }) - - It("passes the list of app GUIDs to the repository", func() { - Expect(serviceBindingRepo.ListServiceBindingsCallCount()).To(Equal(1)) - _, _, message := serviceBindingRepo.ListServiceBindingsArgsForCall(0) - Expect(message.AppGUIDs).To(ConsistOf([]string{"1", "2", "3"})) - }) - }) - - When("a service_instance_guids query parameter is provided", func() { - BeforeEach(func() { - requestPath += "?service_instance_guids=1,2,3" - }) - - It("passes the list of service instance GUIDs to the repository", func() { - Expect(serviceBindingRepo.ListServiceBindingsCallCount()).To(Equal(1)) - _, _, message := serviceBindingRepo.ListServiceBindingsArgsForCall(0) - Expect(message.ServiceInstanceGUIDs).To(ConsistOf([]string{"1", "2", "3"})) - }) - }) - - When("a type query parameter is provided", func() { - BeforeEach(func() { - requestPath += "?type=app" - }) - - It("returns success", func() { - Expect(rr).To(HaveHTTPStatus(http.StatusOK)) - }) - }) - - When("invalid query parameters are provided", func() { + When("decoding URL params fails", func() { BeforeEach(func() { - requestPath += "?foo=bar" + requestValidator.DecodeAndValidateURLValuesReturns(errors.New("boom")) }) - It("returns an Unknown key error", func() { - expectUnknownKeyError("The query parameter is invalid: Valid parameters are: .*") + It("returns an error", func() { + expectUnknownError() }) }) }) @@ -397,19 +316,28 @@ var _ = Describe("ServiceBinding", func() { BeforeEach(func() { requestMethod = "PATCH" requestPath = "/v3/service_credential_bindings/service-binding-guid" - requestBody = `{ - "metadata": { - "labels": { "foo": "bar" }, - "annotations": { "bar": "baz" } - } - }` + requestBody = "doesn't matter" serviceBindingRepo.UpdateServiceBindingReturns(repositories.ServiceBindingRecord{ GUID: "service-binding-guid", }, nil) + + payload := payloads.ServiceBindingUpdate{ + Metadata: payloads.MetadataPatch{ + Labels: map[string]*string{"foo": tools.PtrTo("bar")}, + Annotations: map[string]*string{"bar": tools.PtrTo("baz")}, + }, + } + requestValidator.DecodeAndValidateJSONPayloadStub = decodeAndValidateJSONPayloadStub(&payload) }) It("updates the service binding", func() { + Expect(requestValidator.DecodeAndValidateJSONPayloadCallCount()).To(Equal(1)) + actualReq, _ := requestValidator.DecodeAndValidateJSONPayloadArgsForCall(0) + bodyBytes, err := io.ReadAll(actualReq.Body) + Expect(err).NotTo(HaveOccurred()) + Expect(string(bodyBytes)).To(Equal(requestBody)) + Expect(rr).To(HaveHTTPStatus(http.StatusOK)) Expect(rr).To(HaveHTTPHeaderWithValue("Content-Type", "application/json")) Expect(rr).To(HaveHTTPBody(SatisfyAll( @@ -431,71 +359,11 @@ var _ = Describe("ServiceBinding", func() { When("the payload cannot be decoded", func() { BeforeEach(func() { - requestBody = `{"one":"two"}` + requestValidator.DecodeAndValidateJSONPayloadReturns(errors.New("boom")) }) It("returns an error", func() { - expectUnprocessableEntityError("invalid request body: json: unknown field \"one\"") - }) - }) - - When("a label is invalid", func() { - When("the prefix is cloudfoundry.org", func() { - BeforeEach(func() { - requestBody = `{ - "metadata": { - "labels": { "cloudfoundry.org/test": "production" } - } - }` - }) - - It("returns an unprocessable entity error", func() { - expectUnprocessableEntityError(`Labels and annotations cannot begin with "cloudfoundry.org" or its subdomains`) - }) - }) - - When("the prefix is a subdomain of cloudfoundry.org", func() { - BeforeEach(func() { - requestBody = `{ - "metadata": { - "labels": { "korifi.cloudfoundry.org/test": "production" } - } - }` - }) - - It("returns an unprocessable entity error", func() { - expectUnprocessableEntityError(`Labels and annotations cannot begin with "cloudfoundry.org" or its subdomains`) - }) - }) - }) - - When("an annotation is invalid", func() { - When("the prefix is cloudfoundry.org", func() { - BeforeEach(func() { - requestBody = `{ - "metadata": { - "annotations": { "cloudfoundry.org/test": "production" } - } - }` - }) - - It("returns an unprocessable entity error", func() { - expectUnprocessableEntityError(`Labels and annotations cannot begin with "cloudfoundry.org" or its subdomains`) - }) - }) - - When("the prefix is a subdomain of cloudfoundry.org", func() { - BeforeEach(func() { - requestBody = `{ - "metadata": { - "annotations": { "korifi.cloudfoundry.org/test": "production" } - } - }` - }) - - It("returns an unprocessable entity error", func() { - expectUnprocessableEntityError(`Labels and annotations cannot begin with "cloudfoundry.org" or its subdomains`) - }) + expectUnknownError() }) }) diff --git a/api/payloads/domain_test.go b/api/payloads/domain_test.go index 786766a8f..1b1fb610c 100644 --- a/api/payloads/domain_test.go +++ b/api/payloads/domain_test.go @@ -76,7 +76,7 @@ var _ = Describe("DomainCreate", func() { }) It("returns an appropriate error", func() { - expectUnprocessableEntityError(validatorErr, "data cannot be blank") + expectUnprocessableEntityError(validatorErr, "data is required") }) }) }) diff --git a/api/payloads/relationship.go b/api/payloads/relationship.go index d2205cb2d..d1ce811b3 100644 --- a/api/payloads/relationship.go +++ b/api/payloads/relationship.go @@ -1,7 +1,6 @@ package payloads import ( - payload_validation "code.cloudfoundry.org/korifi/api/payloads/validation" "github.com/jellydator/validation" ) @@ -10,7 +9,9 @@ type Relationship struct { } func (r Relationship) Validate() error { - return validation.ValidateStruct(&r, validation.Field(&r.Data, payload_validation.StrictlyRequired)) + return validation.ValidateStruct(&r, + validation.Field(&r.Data, validation.NotNil), + ) } type RelationshipData struct { @@ -18,5 +19,7 @@ type RelationshipData struct { } func (r RelationshipData) Validate() error { - return validation.ValidateStruct(&r, validation.Field(&r.GUID, payload_validation.StrictlyRequired)) + return validation.ValidateStruct(&r, + validation.Field(&r.GUID, validation.Required), + ) } diff --git a/api/payloads/relationship_test.go b/api/payloads/relationship_test.go index 4dc7cf25b..f676c8cf4 100644 --- a/api/payloads/relationship_test.go +++ b/api/payloads/relationship_test.go @@ -38,7 +38,7 @@ var _ = Describe("Relationship", func() { }) It("returns an appropriate error", func() { - expectUnprocessableEntityError(validatorErr, "data cannot be blank") + expectUnprocessableEntityError(validatorErr, "data is required") }) }) diff --git a/api/payloads/service_binding.go b/api/payloads/service_binding.go index 1bb615481..9a4356941 100644 --- a/api/payloads/service_binding.go +++ b/api/payloads/service_binding.go @@ -4,7 +4,9 @@ import ( "net/url" "code.cloudfoundry.org/korifi/api/payloads/parse" + "code.cloudfoundry.org/korifi/api/payloads/validation" "code.cloudfoundry.org/korifi/api/repositories" + jellidation "github.com/jellydator/validation" ) type ServiceBindingCreate struct { @@ -13,11 +15,6 @@ type ServiceBindingCreate struct { Name *string `json:"name"` } -type ServiceBindingRelationships struct { - App *Relationship `json:"app" validate:"required"` - ServiceInstance *Relationship `json:"service_instance" validate:"required"` -} - func (p ServiceBindingCreate) ToMessage(spaceGUID string) repositories.CreateServiceBindingMessage { return repositories.CreateServiceBindingMessage{ Name: p.Name, @@ -27,6 +24,25 @@ func (p ServiceBindingCreate) ToMessage(spaceGUID string) repositories.CreateSer } } +func (p ServiceBindingCreate) Validate() error { + return jellidation.ValidateStruct(&p, + jellidation.Field(&p.Type, validation.OneOf("app")), + jellidation.Field(&p.Relationships, jellidation.NotNil), + ) +} + +type ServiceBindingRelationships struct { + App *Relationship `json:"app"` + ServiceInstance *Relationship `json:"service_instance"` +} + +func (r ServiceBindingRelationships) Validate() error { + return jellidation.ValidateStruct(&r, + jellidation.Field(&r.App, jellidation.NotNil), + jellidation.Field(&r.ServiceInstance, jellidation.NotNil), + ) +} + type ServiceBindingList struct { AppGUIDs string ServiceInstanceGUIDs string @@ -55,6 +71,12 @@ type ServiceBindingUpdate struct { Metadata MetadataPatch `json:"metadata"` } +func (u ServiceBindingUpdate) Validate() error { + return jellidation.ValidateStruct(&u, + jellidation.Field(&u.Metadata), + ) +} + func (c *ServiceBindingUpdate) ToMessage(serviceBindingGUID string) repositories.UpdateServiceBindingMessage { return repositories.UpdateServiceBindingMessage{ GUID: serviceBindingGUID, diff --git a/api/payloads/service_binding_test.go b/api/payloads/service_binding_test.go index 7b5518748..2e197a207 100644 --- a/api/payloads/service_binding_test.go +++ b/api/payloads/service_binding_test.go @@ -3,9 +3,12 @@ package payloads_test import ( "net/http" + "code.cloudfoundry.org/korifi/api/errors" "code.cloudfoundry.org/korifi/api/payloads" + "code.cloudfoundry.org/korifi/tools" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/onsi/gomega/gstruct" ) var _ = Describe("ServiceBindingList", func() { @@ -25,3 +28,147 @@ var _ = Describe("ServiceBindingList", func() { }) }) }) + +var _ = Describe("ServiceBindingCreate", func() { + var ( + createPayload payloads.ServiceBindingCreate + serviceBindingCreate *payloads.ServiceBindingCreate + validatorErr error + apiError errors.ApiError + ) + + BeforeEach(func() { + serviceBindingCreate = new(payloads.ServiceBindingCreate) + createPayload = payloads.ServiceBindingCreate{ + Relationships: &payloads.ServiceBindingRelationships{ + App: &payloads.Relationship{ + Data: &payloads.RelationshipData{ + GUID: "app-guid", + }, + }, + ServiceInstance: &payloads.Relationship{ + Data: &payloads.RelationshipData{ + GUID: "service-instance-guid", + }, + }, + }, + Type: "app", + } + }) + + JustBeforeEach(func() { + validatorErr = validator.DecodeAndValidateJSONPayload(createRequest(createPayload), serviceBindingCreate) + apiError, _ = validatorErr.(errors.ApiError) + }) + + It("succeeds", func() { + Expect(validatorErr).NotTo(HaveOccurred()) + Expect(serviceBindingCreate).To(gstruct.PointTo(Equal(createPayload))) + }) + + When(`the type is "key"`, func() { + BeforeEach(func() { + createPayload.Type = "key" + }) + + It("fails", func() { + Expect(apiError).To(HaveOccurred()) + Expect(apiError.Detail()).To(ContainSubstring("type value must be one of: app")) + }) + }) + + When("all relationships are missing", func() { + BeforeEach(func() { + createPayload.Relationships = nil + }) + + It("fails", func() { + Expect(apiError).To(HaveOccurred()) + Expect(apiError.Detail()).To(ContainSubstring("relationships is required")) + }) + }) + + When("app relationship is missing", func() { + BeforeEach(func() { + createPayload.Relationships.App = nil + }) + + It("fails", func() { + Expect(apiError).To(HaveOccurred()) + Expect(apiError.Detail()).To(ContainSubstring("relationships.app is required")) + }) + }) + + When("the app GUID is blank", func() { + BeforeEach(func() { + createPayload.Relationships.App.Data.GUID = "" + }) + + It("fails", func() { + Expect(apiError).To(HaveOccurred()) + Expect(apiError.Detail()).To(ContainSubstring("app.data.guid cannot be blank")) + }) + }) + + When("service instance relationship is missing", func() { + BeforeEach(func() { + createPayload.Relationships.ServiceInstance = nil + }) + + It("fails", func() { + Expect(apiError).To(HaveOccurred()) + Expect(apiError.Detail()).To(ContainSubstring("relationships.service_instance is required")) + }) + }) + + When("the service instance GUID is blank", func() { + BeforeEach(func() { + createPayload.Relationships.ServiceInstance.Data.GUID = "" + }) + + It("fails", func() { + Expect(apiError).To(HaveOccurred()) + Expect(apiError.Detail()).To(ContainSubstring("relationships.service_instance.data.guid cannot be blank")) + }) + }) +}) + +var _ = Describe("ServiceBindingUpdate", func() { + var ( + patchPayload payloads.ServiceBindingUpdate + serviceBindingPatch *payloads.ServiceBindingUpdate + validatorErr error + apiError errors.ApiError + ) + + BeforeEach(func() { + serviceBindingPatch = new(payloads.ServiceBindingUpdate) + patchPayload = payloads.ServiceBindingUpdate{ + Metadata: payloads.MetadataPatch{ + Annotations: map[string]*string{"a": tools.PtrTo("av")}, + Labels: map[string]*string{"l": tools.PtrTo("lv")}, + }, + } + }) + + JustBeforeEach(func() { + validatorErr = validator.DecodeAndValidateJSONPayload(createRequest(patchPayload), serviceBindingPatch) + apiError, _ = validatorErr.(errors.ApiError) + }) + + It("succeeds", func() { + Expect(validatorErr).NotTo(HaveOccurred()) + Expect(serviceBindingPatch).To(gstruct.PointTo(Equal(patchPayload))) + }) + + When("metadata uses the cloudfoundry domain", func() { + BeforeEach(func() { + patchPayload.Metadata.Labels["foo.cloudfoundry.org/bar"] = tools.PtrTo("baz") + }) + + It("fails", func() { + Expect(apiError).To(HaveOccurred()) + Expect(apiError.Detail()).To(ContainSubstring("cannot use the cloudfoundry.org domain")) + }) + }) +}) From 9dec9bc11a8e8b9b4b526986ab02195a79c47045 Mon Sep 17 00:00:00 2001 From: Danail Branekov Date: Tue, 27 Jun 2023 09:59:09 +0100 Subject: [PATCH 5/5] Use pointers for relationships E.g. AppCreate payload has relationships.space.data.guid. If we use pointers all the way down, we get the correct error message if the json only contains some of the path. So providing `relationships.space` would error with `data` being required. Issue: https://github.com/cloudfoundry/korifi/issues/1996 Co-authored-by: Kieron Browne --- api/handlers/app_test.go | 4 ++-- api/payloads/app.go | 8 ++++---- api/payloads/app_test.go | 18 ++++++++++++++---- api/payloads/deployment.go | 6 +++--- api/payloads/deployment_test.go | 22 ++++++++++++++++------ api/payloads/lifecycle.go | 16 +++++++--------- api/payloads/lifecycle_test.go | 14 +++++++------- api/payloads/route.go | 4 ++-- 8 files changed, 55 insertions(+), 37 deletions(-) diff --git a/api/handlers/app_test.go b/api/handlers/app_test.go index 93efe6ec1..fa5d0b28d 100644 --- a/api/handlers/app_test.go +++ b/api/handlers/app_test.go @@ -140,8 +140,8 @@ var _ = Describe("App", func() { BeforeEach(func() { payload = &payloads.AppCreate{ Name: appName, - Relationships: payloads.AppRelationships{ - Space: payloads.Relationship{ + Relationships: &payloads.AppRelationships{ + Space: &payloads.Relationship{ Data: &payloads.RelationshipData{ GUID: spaceGUID, }, diff --git a/api/payloads/app.go b/api/payloads/app.go index a7f276c80..7253b8fa1 100644 --- a/api/payloads/app.go +++ b/api/payloads/app.go @@ -22,7 +22,7 @@ var DefaultLifecycleConfig = config.DefaultLifecycleConfig{ type AppCreate struct { Name string `json:"name"` EnvironmentVariables map[string]string `json:"environment_variables"` - Relationships AppRelationships `json:"relationships"` + Relationships *AppRelationships `json:"relationships"` Lifecycle *Lifecycle `json:"lifecycle"` Metadata Metadata `json:"metadata"` } @@ -30,19 +30,19 @@ type AppCreate struct { func (c AppCreate) Validate() error { return validation.ValidateStruct(&c, validation.Field(&c.Name, payload_validation.StrictlyRequired), - validation.Field(&c.Relationships, payload_validation.StrictlyRequired), + validation.Field(&c.Relationships, validation.NotNil), validation.Field(&c.Lifecycle), validation.Field(&c.Metadata), ) } type AppRelationships struct { - Space Relationship `json:"space"` + Space *Relationship `json:"space"` } func (r AppRelationships) Validate() error { return validation.ValidateStruct(&r, - validation.Field(&r.Space, payload_validation.StrictlyRequired), + validation.Field(&r.Space, validation.NotNil), ) } diff --git a/api/payloads/app_test.go b/api/payloads/app_test.go index e4c619ab5..e446231e1 100644 --- a/api/payloads/app_test.go +++ b/api/payloads/app_test.go @@ -40,8 +40,8 @@ var _ = Describe("App payload validation", func() { BeforeEach(func() { payload = payloads.AppCreate{ Name: "my-app", - Relationships: payloads.AppRelationships{ - Space: payloads.Relationship{ + Relationships: &payloads.AppRelationships{ + Space: &payloads.Relationship{ Data: &payloads.RelationshipData{ GUID: "app-guid", }, @@ -83,11 +83,21 @@ var _ = Describe("App payload validation", func() { When("relationships are not set", func() { BeforeEach(func() { - payload.Relationships = payloads.AppRelationships{} + payload.Relationships = nil }) It("returns an unprocessable entity error", func() { - expectUnprocessableEntityError(validatorErr, "relationships cannot be blank") + expectUnprocessableEntityError(validatorErr, "relationships is required") + }) + }) + + When("relationships space is not set", func() { + BeforeEach(func() { + payload.Relationships.Space = nil + }) + + It("returns an unprocessable entity error", func() { + expectUnprocessableEntityError(validatorErr, "relationships.space is required") }) }) diff --git a/api/payloads/deployment.go b/api/payloads/deployment.go index 61264bf72..1aaa57c09 100644 --- a/api/payloads/deployment.go +++ b/api/payloads/deployment.go @@ -9,12 +9,12 @@ type DropletGUID struct { } type DeploymentCreate struct { - Droplet DropletGUID `json:"droplet"` - Relationships DeploymentRelationships `json:"relationships" validate:"required"` + Droplet DropletGUID `json:"droplet"` + Relationships *DeploymentRelationships `json:"relationships" validate:"required"` } type DeploymentRelationships struct { - App Relationship `json:"app" validate:"required"` + App *Relationship `json:"app" validate:"required"` } func (c *DeploymentCreate) ToMessage() repositories.CreateDeploymentMessage { diff --git a/api/payloads/deployment_test.go b/api/payloads/deployment_test.go index d6a1a1cc4..300cd2146 100644 --- a/api/payloads/deployment_test.go +++ b/api/payloads/deployment_test.go @@ -17,8 +17,8 @@ var _ = Describe("DeploymentCreate", func() { Droplet: payloads.DropletGUID{ Guid: "the-droplet", }, - Relationships: payloads.DeploymentRelationships{ - App: payloads.Relationship{ + Relationships: &payloads.DeploymentRelationships{ + App: &payloads.Relationship{ Data: &payloads.RelationshipData{ GUID: "the-app", }, @@ -57,13 +57,23 @@ var _ = Describe("DeploymentCreate", func() { }) }) - When("the app relationship is not specified", func() { + When("the relationship is not specified", func() { BeforeEach(func() { - createDeployment.Relationships = payloads.DeploymentRelationships{} + createDeployment.Relationships = nil }) - It("says app data is required", func() { - expectUnprocessableEntityError(validatorErr, "Data is a required field") + It("says relationships is required", func() { + expectUnprocessableEntityError(validatorErr, "Relationships is a required field") + }) + }) + + When("the relationship app is not specified", func() { + BeforeEach(func() { + createDeployment.Relationships.App = nil + }) + + It("says app is required", func() { + expectUnprocessableEntityError(validatorErr, "App is a required field") }) }) diff --git a/api/payloads/lifecycle.go b/api/payloads/lifecycle.go index b936d622c..4402f363b 100644 --- a/api/payloads/lifecycle.go +++ b/api/payloads/lifecycle.go @@ -1,30 +1,28 @@ package payloads import ( - payload_validation "code.cloudfoundry.org/korifi/api/payloads/validation" "github.com/jellydator/validation" ) type Lifecycle struct { - Type string `json:"type" validate:"required"` - Data LifecycleData `json:"data" validate:"required"` + Type string `json:"type"` + Data *LifecycleData `json:"data"` } func (l Lifecycle) Validate() error { return validation.ValidateStruct(&l, - validation.Field(&l.Type, payload_validation.StrictlyRequired), - validation.Field(&l.Data, payload_validation.StrictlyRequired), + validation.Field(&l.Type, validation.Required), + validation.Field(&l.Data, validation.NotNil), ) } type LifecycleData struct { - Buildpacks []string `json:"buildpacks" validate:"required"` - Stack string `json:"stack" validate:"required"` + Buildpacks []string `json:"buildpacks"` + Stack string `json:"stack"` } func (d LifecycleData) Validate() error { return validation.ValidateStruct(&d, - validation.Field(&d.Buildpacks, payload_validation.StrictlyRequired), - validation.Field(&d.Stack, payload_validation.StrictlyRequired), + validation.Field(&d.Stack, validation.Required), ) } diff --git a/api/payloads/lifecycle_test.go b/api/payloads/lifecycle_test.go index 400449b50..bd967aa44 100644 --- a/api/payloads/lifecycle_test.go +++ b/api/payloads/lifecycle_test.go @@ -22,7 +22,7 @@ var _ = Describe("Lifecycle", func() { payload = payloads.Lifecycle{ Type: "buildpack", - Data: payloads.LifecycleData{ + Data: &payloads.LifecycleData{ Buildpacks: []string{"foo", "bar"}, Stack: "baz", }, @@ -50,23 +50,23 @@ var _ = Describe("Lifecycle", func() { }) }) - When("stack is not set", func() { + When("lifecycle data is not set", func() { BeforeEach(func() { - payload.Data.Stack = "" + payload.Data = nil }) It("returns an appropriate error", func() { - expectUnprocessableEntityError(validatorErr, "data.stack cannot be blank") + expectUnprocessableEntityError(validatorErr, "data is required") }) }) - When("buildpacks are not set", func() { + When("stack is not set", func() { BeforeEach(func() { - payload.Data.Buildpacks = nil + payload.Data.Stack = "" }) It("returns an appropriate error", func() { - expectUnprocessableEntityError(validatorErr, "data.buildpacks cannot be blank") + expectUnprocessableEntityError(validatorErr, "data.stack cannot be blank") }) }) }) diff --git a/api/payloads/route.go b/api/payloads/route.go index c2fb2134a..a9c2394a9 100644 --- a/api/payloads/route.go +++ b/api/payloads/route.go @@ -39,8 +39,8 @@ func (p RouteCreate) ToMessage(domainNamespace, domainName string) repositories. } type RouteRelationships struct { - Domain Relationship `json:"domain" validate:"required"` - Space Relationship `json:"space" validate:"required"` + Domain Relationship `json:"domain"` + Space Relationship `json:"space"` } func (r RouteRelationships) Validate() error {