Skip to content

Commit

Permalink
Move role api param testing to payloads package
Browse files Browse the repository at this point in the history 
We also migrated all role validation to jellydator

Issue: #1996
Co-authored-by: Danail Branekov <[email protected]>
  • Loading branch information
@danail-branekov
Kieron Browne and danail-branekov committed Jun 23, 2023
1 parent ddb6878 commit e99b618
Show file tree
Hide file tree
Showing 6 changed files with 199 additions and 220 deletions.
36 changes: 8 additions & 28 deletions api/handlers/role.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,
}
}

Expand All @@ -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")
}

Expand All @@ -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")
}
Expand All @@ -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) &&
Expand Down
74 changes: 38 additions & 36 deletions api/handlers/role_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package handlers_test

import (
"errors"
"io"
"net/http"
"net/http/httptest"
"strings"
Expand Down Expand Up @@ -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",
},
Expand All @@ -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))
Expand Down Expand Up @@ -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))
Expand All @@ -178,25 +187,14 @@ 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"),
MatchJSONPath("$.resources[1].guid", "role-2"),
)))
})

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{
Expand All @@ -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)

Expand All @@ -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()
})
})

Expand Down
60 changes: 0 additions & 60 deletions api/handlers/validator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}

Expand All @@ -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 {
Expand Down
Loading

0 comments on commit e99b618

Please sign in to comment.