From ebe07a5d9b2badcdeb4616a0ccd5d753374fedac Mon Sep 17 00:00:00 2001 From: barco Date: Wed, 17 Apr 2024 15:59:04 +0200 Subject: [PATCH 01/17] fix: add back URL Param validation from previous commit I mistakenly dropped this change in my previous PR 275 --- pkg/clients/handlers.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/clients/handlers.go b/pkg/clients/handlers.go index bc8a2a4a0..47a856171 100644 --- a/pkg/clients/handlers.go +++ b/pkg/clients/handlers.go @@ -27,9 +27,9 @@ type API struct { func (a *API) RegisterEndpoints(mux *chi.Mux) { mux.Get("/api/v0/clients", a.ListClients) mux.Post("/api/v0/clients", a.CreateClient) - mux.Get("/api/v0/clients/{id}", a.GetClient) - mux.Put("/api/v0/clients/{id}", a.UpdateClient) - mux.Delete("/api/v0/clients/{id}", a.DeleteClient) + mux.Get("/api/v0/clients/{id:.+}", a.GetClient) + mux.Put("/api/v0/clients/{id:.+}", a.UpdateClient) + mux.Delete("/api/v0/clients/{id:.+}", a.DeleteClient) } func (a *API) RegisterValidation(v validation.ValidationRegistryInterface) { From 3d479919a3a799161879e7211bd70264d62c8217 Mon Sep 17 00:00:00 2001 From: barco Date: Wed, 17 Apr 2024 16:00:49 +0200 Subject: [PATCH 02/17] refactor: receiver name --- internal/validation/registry_test.go | 4 ++-- pkg/groups/validation.go | 2 +- pkg/identities/validation.go | 6 +++--- pkg/schemas/validation.go | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/validation/registry_test.go b/internal/validation/registry_test.go index ef48b9f55..72c4908f2 100644 --- a/internal/validation/registry_test.go +++ b/internal/validation/registry_test.go @@ -22,7 +22,7 @@ import ( type payloadValidator struct{} -func (_ *payloadValidator) Validate(ctx context.Context, _, _ string, _ []byte) (context.Context, validator.ValidationErrors, error) { +func (p *payloadValidator) Validate(ctx context.Context, _, _ string, _ []byte) (context.Context, validator.ValidationErrors, error) { e := mockValidationErrors() if e == nil { return ctx, nil, nil @@ -30,7 +30,7 @@ func (_ *payloadValidator) Validate(ctx context.Context, _, _ string, _ []byte) return ctx, e, nil } -func (_ *payloadValidator) NeedsValidation(r *http.Request) bool { +func (p *payloadValidator) NeedsValidation(r *http.Request) bool { return true } diff --git a/pkg/groups/validation.go b/pkg/groups/validation.go index e37dd08e4..a2e18eea2 100644 --- a/pkg/groups/validation.go +++ b/pkg/groups/validation.go @@ -19,7 +19,7 @@ type PayloadValidator struct { validator *validator.Validate } -func (_ *PayloadValidator) NeedsValidation(r *http.Request) bool { +func (p *PayloadValidator) NeedsValidation(r *http.Request) bool { return r.Method == http.MethodPost || r.Method == http.MethodPatch } diff --git a/pkg/identities/validation.go b/pkg/identities/validation.go index 1156a060f..b52465a25 100644 --- a/pkg/identities/validation.go +++ b/pkg/identities/validation.go @@ -39,7 +39,7 @@ func (p *PayloadValidator) setupValidator() { p.validator.RegisterStructValidationMapRules(credentialsRules, UpdateIdentityRequest{}.UpdateIdentityBody.Credentials) } -func (_ *PayloadValidator) NeedsValidation(req *http.Request) bool { +func (p *PayloadValidator) NeedsValidation(req *http.Request) bool { return req.Method == http.MethodPost || req.Method == http.MethodPut } @@ -78,11 +78,11 @@ func (p *PayloadValidator) Validate(ctx context.Context, method, endpoint string return ctx, err.(validator.ValidationErrors), nil } -func (_ *PayloadValidator) isCreateIdentity(method, endpoint string) bool { +func (p *PayloadValidator) isCreateIdentity(method, endpoint string) bool { return endpoint == "" && method == http.MethodPost } -func (_ *PayloadValidator) isUpdateIdentity(method, endpoint string) bool { +func (p *PayloadValidator) isUpdateIdentity(method, endpoint string) bool { return strings.HasPrefix(endpoint, "/") && method == http.MethodPut } diff --git a/pkg/schemas/validation.go b/pkg/schemas/validation.go index da625a564..075fa0278 100644 --- a/pkg/schemas/validation.go +++ b/pkg/schemas/validation.go @@ -33,7 +33,7 @@ func (p *PayloadValidator) setupValidator() { ) } -func (_ *PayloadValidator) NeedsValidation(r *http.Request) bool { +func (p *PayloadValidator) NeedsValidation(r *http.Request) bool { return r.Method == http.MethodPost || r.Method == http.MethodPut || r.Method == http.MethodPatch } From 8a35a63ec85356098876fd25a77bc430146eff07 Mon Sep 17 00:00:00 2001 From: barco Date: Thu, 18 Apr 2024 10:24:16 +0200 Subject: [PATCH 03/17] refactor: improve error logging --- pkg/clients/handlers.go | 2 +- pkg/groups/handlers.go | 2 +- pkg/identities/handlers.go | 2 +- pkg/idp/handlers.go | 2 +- pkg/roles/handlers.go | 2 +- pkg/rules/handlers.go | 2 +- pkg/schemas/handlers.go | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/clients/handlers.go b/pkg/clients/handlers.go index 47a856171..b4d800b2c 100644 --- a/pkg/clients/handlers.go +++ b/pkg/clients/handlers.go @@ -35,7 +35,7 @@ func (a *API) RegisterEndpoints(mux *chi.Mux) { func (a *API) RegisterValidation(v validation.ValidationRegistryInterface) { err := v.RegisterPayloadValidator(a.apiKey, a.payloadValidator) if err != nil { - a.logger.Fatalf("unexpected validatingFunc already registered for clients, %s", err) + a.logger.Fatalf("unexpected error while registering PayloadValidator for 'clients', %s", err) } } diff --git a/pkg/groups/handlers.go b/pkg/groups/handlers.go index 2145f9247..09d9d7497 100644 --- a/pkg/groups/handlers.go +++ b/pkg/groups/handlers.go @@ -79,7 +79,7 @@ func (a *API) RegisterEndpoints(mux *chi.Mux) { func (a *API) RegisterValidation(v validation.ValidationRegistryInterface) { err := v.RegisterPayloadValidator(a.apiKey, a.payloadValidator) if err != nil { - a.logger.Fatalf("unexpected validatingFunc already registered for groups, %s", err) + a.logger.Fatalf("unexpected error while registering PayloadValidator for groups, %s", err) } } diff --git a/pkg/identities/handlers.go b/pkg/identities/handlers.go index 350704e3c..99e29f40e 100644 --- a/pkg/identities/handlers.go +++ b/pkg/identities/handlers.go @@ -49,7 +49,7 @@ func (a *API) RegisterValidation(v validation.ValidationRegistryInterface) { err := v.RegisterPayloadValidator(a.apiKey, a.payloadValidator) if err != nil { - a.logger.Fatalf("unexpected validatingFunc already registered for identities, %s", err) + a.logger.Fatalf("unexpected error while registering PayloadValidator for identities, %s", err) } } diff --git a/pkg/idp/handlers.go b/pkg/idp/handlers.go index e914295f4..ca49c88eb 100644 --- a/pkg/idp/handlers.go +++ b/pkg/idp/handlers.go @@ -36,7 +36,7 @@ func (a *API) RegisterEndpoints(mux *chi.Mux) { func (a *API) RegisterValidation(v validation.ValidationRegistryInterface) { err := v.RegisterPayloadValidator(a.apiKey, a.payloadValidator) if err != nil { - a.logger.Fatalf("unexpected validatingFunc already registered for idps, %s", err) + a.logger.Fatalf("unexpected error while registering PayloadValidator for idps, %s", err) } } diff --git a/pkg/roles/handlers.go b/pkg/roles/handlers.go index 088e5d6e3..7065b1cc1 100644 --- a/pkg/roles/handlers.go +++ b/pkg/roles/handlers.go @@ -64,7 +64,7 @@ func (a *API) RegisterEndpoints(mux *chi.Mux) { func (a *API) RegisterValidation(v validation.ValidationRegistryInterface) { err := v.RegisterPayloadValidator("roles", a.payloadValidator) if err != nil { - a.logger.Fatalf("unexpected validatingFunc already registered for roles, %s", err) + a.logger.Fatalf("unexpected error while registering PayloadValidator for roles, %s", err) } } diff --git a/pkg/rules/handlers.go b/pkg/rules/handlers.go index f56460d4a..be4557c34 100644 --- a/pkg/rules/handlers.go +++ b/pkg/rules/handlers.go @@ -36,7 +36,7 @@ func (a *API) RegisterEndpoints(mux *chi.Mux) { func (a *API) RegisterValidation(v validation.ValidationRegistryInterface) { err := v.RegisterPayloadValidator(a.apiKey, a.payloadValidator) if err != nil { - a.logger.Fatalf("unexpected validatingFunc already registered for rules, %s", err) + a.logger.Fatalf("unexpected error while registering PayloadValidator for rules, %s", err) } } diff --git a/pkg/schemas/handlers.go b/pkg/schemas/handlers.go index 08283b046..b3b86e520 100644 --- a/pkg/schemas/handlers.go +++ b/pkg/schemas/handlers.go @@ -37,7 +37,7 @@ func (a *API) RegisterEndpoints(mux *chi.Mux) { func (a *API) RegisterValidation(v validation.ValidationRegistryInterface) { err := v.RegisterPayloadValidator(a.apiKey, a.payloadValidator) if err != nil { - a.logger.Fatalf("unexpected validatingFunc already registered for schemas, %s", err) + a.logger.Fatalf("unexpected error while registering PayloadValidator for schemas, %s", err) } } From 549d985ed5ded7f8b1522479208d1637bb5e6855 Mon Sep 17 00:00:00 2001 From: barco Date: Thu, 18 Apr 2024 09:37:12 +0200 Subject: [PATCH 04/17] feat: add validation implementation for `clients` --- pkg/clients/handlers.go | 2 +- pkg/clients/validation.go | 84 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 pkg/clients/validation.go diff --git a/pkg/clients/handlers.go b/pkg/clients/handlers.go index b4d800b2c..0c0691c56 100644 --- a/pkg/clients/handlers.go +++ b/pkg/clients/handlers.go @@ -202,7 +202,7 @@ func NewAPI(service ServiceInterface, logger logging.LoggerInterface) *API { a.apiKey = "clients" a.service = service - //a.payloadValidator = NewClientsPayloadValidator(a.apiKey) + a.payloadValidator = NewClientsPayloadValidator(a.apiKey) a.logger = logger return a diff --git a/pkg/clients/validation.go b/pkg/clients/validation.go new file mode 100644 index 000000000..db9c392b6 --- /dev/null +++ b/pkg/clients/validation.go @@ -0,0 +1,84 @@ +// Copyright 2024 Canonical Ltd. +// SPDX-License-Identifier: AGPL-3.0 + +package clients + +import ( + "context" + "encoding/json" + "net/http" + "strings" + + "github.com/go-playground/validator/v10" + client "github.com/ory/hydra-client-go/v2" + + "github.com/canonical/identity-platform-admin-ui/internal/validation" +) + +var ( + oauth2ClientRules = map[string]string{ + "AllowedCorsOrigins": "omitempty,dive,required", + "Audience": "omitempty,dive,required", + "ClientName": "required", + "SubjectType": "omitempty,oneof=pairwise public", + "GrantTypes": "omitempty,dive,required", + "TokenEndpointAuthMethod": "omitempty,oneof=client_secret_basic client_secret_post private_key_jwt none", + } +) + +type PayloadValidator struct { + apiKey string + validator *validator.Validate +} + +func (p *PayloadValidator) setupValidator() { + p.validator.RegisterStructValidationMapRules(oauth2ClientRules, client.OAuth2Client{}) +} + +func (p *PayloadValidator) NeedsValidation(req *http.Request) bool { + return req.Method == http.MethodPost || req.Method == http.MethodPut +} + +func (p *PayloadValidator) Validate(ctx context.Context, method, endpoint string, body []byte) (context.Context, validator.ValidationErrors, error) { + validated := false + var err error + + if p.isCreateClient(method, endpoint) || p.isUpdateClient(method, endpoint) { + clientRequest := new(client.OAuth2Client) + if err := json.Unmarshal(body, clientRequest); err != nil { + return ctx, nil, err + } + + err = p.validator.Struct(clientRequest) + validated = true + + } + + if !validated { + return ctx, nil, validation.NoMatchError(p.apiKey) + } + + if err == nil { + return ctx, nil, nil + } + + return ctx, err.(validator.ValidationErrors), nil +} + +func (p *PayloadValidator) isCreateClient(method string, endpoint string) bool { + return method == http.MethodPost && endpoint == "" +} + +func (p *PayloadValidator) isUpdateClient(method string, endpoint string) bool { + return method == http.MethodPut && strings.HasPrefix(endpoint, "/") +} + +func NewClientsPayloadValidator(apiKey string) *PayloadValidator { + p := new(PayloadValidator) + p.apiKey = apiKey + p.validator = validation.NewValidator() + + p.setupValidator() + + return p +} From 95369d958653a520a05995c031067198477098d4 Mon Sep 17 00:00:00 2001 From: barco Date: Thu, 18 Apr 2024 09:37:28 +0200 Subject: [PATCH 05/17] test: validation implementation for `clients` --- pkg/clients/handlers_test.go | 26 ++++ pkg/clients/validation_test.go | 214 +++++++++++++++++++++++++++++++++ 2 files changed, 240 insertions(+) create mode 100644 pkg/clients/validation_test.go diff --git a/pkg/clients/handlers_test.go b/pkg/clients/handlers_test.go index ff3db4e60..605272123 100644 --- a/pkg/clients/handlers_test.go +++ b/pkg/clients/handlers_test.go @@ -21,6 +21,7 @@ import ( //go:generate mockgen -build_flags=--mod=mod -package clients -destination ./mock_logger.go -source=../../internal/logging/interfaces.go //go:generate mockgen -build_flags=--mod=mod -package clients -destination ./mock_clients.go -source=./interfaces.go +//go:generate mockgen -build_flags=--mod=mod -package clients -destination ./mock_validation.go -source=../../internal/validation/registry.go func TestHandleGetClientSuccess(t *testing.T) { ctrl := gomock.NewController(t) @@ -573,3 +574,28 @@ func TestHandleListClientServiceError(t *testing.T) { t.Fatalf("expected data to be %+v, got: %+v", expectedData, rr) } } + +func TestRegisterValidation(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockLogger := NewMockLoggerInterface(ctrl) + mockService := NewMockServiceInterface(ctrl) + mockValidationRegistry := NewMockValidationRegistryInterface(ctrl) + + apiKey := "clients" + mockValidationRegistry.EXPECT(). + RegisterPayloadValidator(gomock.Eq(apiKey), gomock.Any()). + Return(nil) + mockValidationRegistry.EXPECT(). + RegisterPayloadValidator(gomock.Eq(apiKey), gomock.Any()). + Return(fmt.Errorf("key is already registered")) + + // first registration of `apiKey` is successful + NewAPI(mockService, mockLogger).RegisterValidation(mockValidationRegistry) + + mockLogger.EXPECT().Fatal(gomock.Any()).Times(1) + + // second registration of `apiKey` causes logger.Fatal invocation + NewAPI(mockService, mockLogger).RegisterValidation(mockValidationRegistry) +} diff --git a/pkg/clients/validation_test.go b/pkg/clients/validation_test.go new file mode 100644 index 000000000..f563d6a42 --- /dev/null +++ b/pkg/clients/validation_test.go @@ -0,0 +1,214 @@ +// Copyright 2024 Canonical Ltd. +// SPDX-License-Identifier: AGPL-3.0 + +package clients + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-playground/validator/v10" + client "github.com/ory/kratos-client-go" + + "github.com/canonical/identity-platform-admin-ui/internal/validation" +) + +func TestNeedsValidation(t *testing.T) { + p := new(PayloadValidator) + p.validator = validation.NewValidator() + p.setupValidator() + + for _, tt := range []struct { + name string + req *http.Request + expectedResult bool + }{ + { + name: http.MethodPost, + req: httptest.NewRequest(http.MethodPost, "/", nil), + expectedResult: true, + }, + { + name: http.MethodPut, + req: httptest.NewRequest(http.MethodPut, "/", nil), + expectedResult: true, + }, + { + name: http.MethodGet, + req: httptest.NewRequest(http.MethodGet, "/", nil), + expectedResult: false, + }, + { + name: http.MethodPatch, + req: httptest.NewRequest(http.MethodPatch, "/", nil), + expectedResult: false, + }, + { + name: http.MethodDelete, + req: httptest.NewRequest(http.MethodDelete, "/", nil), + expectedResult: false, + }, + { + name: http.MethodConnect, + req: httptest.NewRequest(http.MethodConnect, "/", nil), + expectedResult: false, + }, + { + name: http.MethodHead, + req: httptest.NewRequest(http.MethodHead, "/", nil), + expectedResult: false, + }, + { + name: http.MethodTrace, + req: httptest.NewRequest(http.MethodTrace, "/", nil), + expectedResult: false, + }, + { + name: http.MethodOptions, + req: httptest.NewRequest(http.MethodOptions, "/", nil), + expectedResult: false, + }, + } { + tt := tt + t.Run(tt.name, func(t *testing.T) { + result := p.NeedsValidation(tt.req) + + if result != tt.expectedResult { + t.Fatalf("Result doesn't match expected one, obtained %t instead of %t", result, tt.expectedResult) + } + }) + } + +} + +func TestValidate(t *testing.T) { + p := new(PayloadValidator) + p.apiKey = "clients" + p.validator = validation.NewValidator() + p.setupValidator() + + for _, tt := range []struct { + name string + method string + endpoint string + body func() []byte + expectedResult validator.ValidationErrors + expectedError error + }{ + { + name: "CreateClientSuccess", + method: http.MethodPost, + endpoint: "", + body: func() []byte { + clientName := "mock-name" + subjectType := "pairwise" + tokenAuthMethod := "client_secret_basic" + c := client.OAuth2Client{ + AllowedCorsOrigins: []string{"mock-origin-1", "mock-origin-2"}, + Audience: []string{"mock-aud-1", "mock-aud-2"}, + ClientName: &clientName, + SubjectType: &subjectType, + GrantTypes: []string{"code_grant"}, + TokenEndpointAuthMethod: &tokenAuthMethod, + } + + marshal, _ := json.Marshal(c) + return marshal + }, + expectedResult: nil, + expectedError: nil, + }, + { + name: "UpdateClientSuccess", + method: http.MethodPut, + endpoint: "/client-id", + body: func() []byte { + clientName := "mock-name" + subjectType := "pairwise" + tokenAuthMethod := "client_secret_basic" + c := client.OAuth2Client{ + AllowedCorsOrigins: []string{"mock-origin-1", "mock-origin-2"}, + Audience: []string{"mock-aud-1", "mock-aud-2"}, + ClientName: &clientName, + SubjectType: &subjectType, + GrantTypes: []string{}, + TokenEndpointAuthMethod: &tokenAuthMethod, + } + + marshal, _ := json.Marshal(c) + return marshal + }, + expectedResult: nil, + expectedError: nil, + }, + { + name: "NoMatch", + method: http.MethodPost, + endpoint: "no-match-endpoint", + body: func() []byte { + return nil + }, + expectedResult: nil, + expectedError: validation.NoMatchError(p.apiKey), + }, + { + name: "CreateClientFailure", + method: http.MethodPost, + endpoint: "", + body: func() []byte { + subjectType := "not-one-of-allowed-values" + tokenAuthMethod := "not-one-of-allowed-values" + c := client.OAuth2Client{ + AllowedCorsOrigins: []string{"", "mock-origin-2"}, + Audience: []string{"mock-aud-1", ""}, + SubjectType: &subjectType, + GrantTypes: []string{}, + TokenEndpointAuthMethod: &tokenAuthMethod, + } + + marshal, _ := json.Marshal(c) + return marshal + }, + expectedResult: validator.ValidationErrors{}, + expectedError: nil, + }, + { + name: "UpdateClientFailure", + method: http.MethodPut, + endpoint: "/client-id", + body: func() []byte { + subjectType := "not-one-of-allowed-values" + tokenAuthMethod := "not-one-of-allowed-values" + c := client.OAuth2Client{ + AllowedCorsOrigins: []string{"", "mock-origin-2"}, + Audience: []string{"mock-aud-1", ""}, + SubjectType: &subjectType, + GrantTypes: []string{}, + TokenEndpointAuthMethod: &tokenAuthMethod, + } + + marshal, _ := json.Marshal(c) + return marshal + }, + expectedResult: validator.ValidationErrors{}, + expectedError: nil, + }, + } { + tt := tt + t.Run(tt.name, func(t *testing.T) { + _, result, err := p.Validate(context.TODO(), tt.method, tt.endpoint, tt.body()) + + if err != nil && err.Error() != tt.expectedError.Error() { + t.Fatalf("Returned error doesn't match expected, obtained '%v' instead of '%v'", err, tt.expectedError) + } else if result != nil && errors.Is(result, tt.expectedResult) { + t.Fatalf("Returned validation errors don't match expected, obtained '%v' instead of '%v'", result, tt.expectedResult) + } else { + return + } + }) + } +} From ab8652f13c437cf64ca6978726b252059f4fb324 Mon Sep 17 00:00:00 2001 From: barco Date: Thu, 18 Apr 2024 16:00:43 +0200 Subject: [PATCH 06/17] fix(schemas): validation and improved tests --- pkg/schemas/validation.go | 12 +++++--- pkg/schemas/validation_test.go | 55 ++++++++++++++++++++++++++-------- 2 files changed, 50 insertions(+), 17 deletions(-) diff --git a/pkg/schemas/validation.go b/pkg/schemas/validation.go index 075fa0278..c2e740e64 100644 --- a/pkg/schemas/validation.go +++ b/pkg/schemas/validation.go @@ -37,19 +37,23 @@ func (p *PayloadValidator) NeedsValidation(r *http.Request) bool { return r.Method == http.MethodPost || r.Method == http.MethodPut || r.Method == http.MethodPatch } +func (p *PayloadValidator) isCreateSchema(method, endpoint string) bool { + return endpoint == "" && method == http.MethodPost +} + func (p *PayloadValidator) isPartialUpdate(method, endpoint string) bool { return strings.HasPrefix(endpoint, "/") && method == http.MethodPatch } -func (p *PayloadValidator) isCreateOrUpdateSchema(method, endpoint string) bool { - return (endpoint == "" && method == http.MethodPost) || (endpoint == "/default" && method == http.MethodPut) +func (p *PayloadValidator) isUpdateDefaultSchema(method, endpoint string) bool { + return endpoint == "/default" && method == http.MethodPut } func (p *PayloadValidator) Validate(ctx context.Context, method, endpoint string, body []byte) (context.Context, validator.ValidationErrors, error) { validated := false var err error - if p.isCreateOrUpdateSchema(method, endpoint) { + if p.isCreateSchema(method, endpoint) || p.isPartialUpdate(method, endpoint) { schema := new(kClient.IdentitySchemaContainer) if err := json.Unmarshal(body, schema); err != nil { return ctx, nil, err @@ -59,7 +63,7 @@ func (p *PayloadValidator) Validate(ctx context.Context, method, endpoint string validated = true } - if p.isPartialUpdate(method, endpoint) { + if p.isUpdateDefaultSchema(method, endpoint) { schema := new(DefaultSchema) if err := json.Unmarshal(body, schema); err != nil { return ctx, nil, err diff --git a/pkg/schemas/validation_test.go b/pkg/schemas/validation_test.go index efc9bbf42..16fd93e48 100644 --- a/pkg/schemas/validation_test.go +++ b/pkg/schemas/validation_test.go @@ -125,7 +125,7 @@ func TestValidate(t *testing.T) { expectedError error }{ { - name: "CreateOrUpdateSchemaSuccessCreate", + name: "CreateSchemaSuccessCreate", method: http.MethodPost, endpoint: "", body: func() []byte { @@ -139,14 +139,13 @@ func TestValidate(t *testing.T) { expectedError: nil, }, { - name: "CreateOrUpdateSchemaSuccessUpdateDefault", + name: "UpdateDefaultSchemaSuccess", method: http.MethodPut, endpoint: "/default", body: func() []byte { id := "default" - updateRequest := new(kClient.IdentitySchemaContainer) - updateRequest.Schema = mockSchema - updateRequest.Id = &id + updateRequest := new(DefaultSchema) + updateRequest.ID = id marshal, _ := json.Marshal(updateRequest) return marshal @@ -181,7 +180,7 @@ func TestValidate(t *testing.T) { expectedError: validation.NoMatchError(p.apiKey), }, { - name: "CreateOrUpdateSchemaFailure", + name: "CreateSchemaFailure", method: http.MethodPost, endpoint: "", body: func() []byte { @@ -197,8 +196,8 @@ func TestValidate(t *testing.T) { }, { name: "PartialUpdateSchemaFailure", - method: http.MethodPatch, - endpoint: "/mock-id", + method: http.MethodPost, + endpoint: "", body: func() []byte { id := "mock-id" updateRequest := new(kClient.IdentitySchemaContainer) @@ -210,16 +209,46 @@ func TestValidate(t *testing.T) { expectedResult: validator.ValidationErrors{}, expectedError: nil, }, + { + name: "UpdateDefaultSchemaFailure", + method: http.MethodPatch, + endpoint: "/mock-id", + body: func() []byte { + updateRequest := new(DefaultSchema) + updateRequest.ID = "mock-id" + + marshal, _ := json.Marshal(updateRequest) + return marshal + }, + expectedResult: validator.ValidationErrors{}, + expectedError: nil, + }, } { tt := tt t.Run(tt.name, func(t *testing.T) { _, result, err := p.Validate(context.TODO(), tt.method, tt.endpoint, tt.body()) - if err != nil && err.Error() != tt.expectedError.Error() { - t.Fatalf("Returned error doesn't match expected, obtained '%v' instead of '%v'", err, tt.expectedError) - } else if result != nil && errors.Is(result, tt.expectedResult) { - t.Fatalf("Returned validation errors don't match expected, obtained '%v' instead of '%v'", result, tt.expectedResult) - } else { + if err != nil { + if tt.expectedError == nil { + t.Fatalf("Unexpected error for '%s'", tt.name) + } + + if err.Error() != tt.expectedError.Error() { + t.Fatalf("Returned error doesn't match expected, obtained '%v' instead of '%v'", err, tt.expectedError) + } + + return + } + + if result != nil { + if tt.expectedResult == nil { + t.Fatalf("Unexpected result for '%s'", tt.name) + } + + if errors.Is(result, tt.expectedResult) { + t.Fatalf("Returned validation errors don't match expected, obtained '%v' instead of '%v'", result, tt.expectedResult) + } + return } }) From c42bd45cf7af8b1fe46858c8480693bda8dc9145 Mon Sep 17 00:00:00 2001 From: barco Date: Thu, 18 Apr 2024 16:02:35 +0200 Subject: [PATCH 07/17] feat(rules): add validation implementation --- pkg/rules/handlers.go | 2 +- pkg/rules/validation.go | 92 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-) create mode 100644 pkg/rules/validation.go diff --git a/pkg/rules/handlers.go b/pkg/rules/handlers.go index be4557c34..74cdac4b8 100644 --- a/pkg/rules/handlers.go +++ b/pkg/rules/handlers.go @@ -250,7 +250,7 @@ func (a *API) handleRemove(w http.ResponseWriter, r *http.Request) { func NewAPI(service ServiceInterface, logger logging.LoggerInterface) *API { a := new(API) a.apiKey = "rules" - + a.payloadValidator = NewRulesPayloadValidator(a.apiKey) a.service = service a.logger = logger diff --git a/pkg/rules/validation.go b/pkg/rules/validation.go new file mode 100644 index 000000000..537bee5b9 --- /dev/null +++ b/pkg/rules/validation.go @@ -0,0 +1,92 @@ +// Copyright 2024 Canonical Ltd. +// SPDX-License-Identifier: AGPL-3.0 + +package rules + +import ( + "context" + "encoding/json" + "net/http" + "strings" + + "github.com/go-playground/validator/v10" + oathkeeper "github.com/ory/oathkeeper-client-go" + + "github.com/canonical/identity-platform-admin-ui/internal/validation" +) + +var ( + ruleRules = map[string]string{ + "Authenticators": "required,min=1,dive,required", + "Authorizer": "required", + "Match": "required", + "Mutators": "omitempty,dive,required", + "Upstream": "omitempty,required", + } + + ruleMatchRules = map[string]string{ + "Methods": "required,min=1,dive,httpmethod", + "Url": "required", + } + + ruleHandlerRules = map[string]string{ + "Handler": "required", + } +) + +type PayloadValidator struct { + apiKey string + validator *validator.Validate +} + +func (p *PayloadValidator) setupValidator() { + p.validator.RegisterAlias("httpmethod", "oneof=GET HEAD POST PUT PATCH DELETE CONNECT OPTIONS TRACE") + + p.validator.RegisterStructValidationMapRules(ruleRules, oathkeeper.Rule{}) + p.validator.RegisterStructValidationMapRules(ruleMatchRules, oathkeeper.RuleMatch{}) + p.validator.RegisterStructValidationMapRules(ruleHandlerRules, oathkeeper.RuleHandler{}) +} + +func (p *PayloadValidator) NeedsValidation(req *http.Request) bool { + return req.Method == http.MethodPost || req.Method == http.MethodPut +} + +func (p *PayloadValidator) Validate(ctx context.Context, method, endpoint string, body []byte) (context.Context, validator.ValidationErrors, error) { + validated := false + var err error + + if p.isCreateOrUpdateRule(method, endpoint) { + ruleRequest := new(oathkeeper.Rule) + if err := json.Unmarshal(body, ruleRequest); err != nil { + return ctx, nil, err + } + + err = p.validator.Struct(ruleRequest) + validated = true + + } + + if !validated { + return ctx, nil, validation.NoMatchError(p.apiKey) + } + + if err == nil { + return ctx, nil, nil + } + + return ctx, err.(validator.ValidationErrors), nil +} + +func (p *PayloadValidator) isCreateOrUpdateRule(method string, endpoint string) bool { + return (method == http.MethodPost && endpoint == "") || (method == http.MethodPut && strings.HasPrefix(endpoint, "/")) +} + +func NewRulesPayloadValidator(apiKey string) *PayloadValidator { + p := new(PayloadValidator) + p.apiKey = apiKey + p.validator = validation.NewValidator() + + p.setupValidator() + + return p +} From 1f94d0a6bfd44a7ee583919f180f0fc15cb620ff Mon Sep 17 00:00:00 2001 From: barco Date: Thu, 18 Apr 2024 16:03:01 +0200 Subject: [PATCH 08/17] test(rules): validation --- pkg/rules/handlers_test.go | 28 +++- pkg/rules/validation_test.go | 249 +++++++++++++++++++++++++++++++++++ 2 files changed, 276 insertions(+), 1 deletion(-) create mode 100644 pkg/rules/validation_test.go diff --git a/pkg/rules/handlers_test.go b/pkg/rules/handlers_test.go index acaec0b70..0e2446249 100644 --- a/pkg/rules/handlers_test.go +++ b/pkg/rules/handlers_test.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical Ltd +// Copyright 2024 Canonical Ltd. // SPDX-License-Identifier: AGPL-3.0 package rules @@ -25,6 +25,7 @@ import ( //go:generate mockgen -build_flags=--mod=mod -package rules -destination ./mock_tracing.go go.opentelemetry.io/otel/trace Tracer //go:generate mockgen -build_flags=--mod=mod -package rules -destination ./mock_corev1.go k8s.io/client-go/kubernetes/typed/core/v1 CoreV1Interface,ConfigMapInterface //go:generate mockgen -build_flags=--mod=mod -package rules -destination ./mock_oathkeeper.go github.com/ory/oathkeeper-client-go ApiApi +//go:generate mockgen -build_flags=--mod=mod -package rules -destination ./mock_validation.go -source=../../internal/validation/registry.go func TestHandleListSuccess(t *testing.T) { ctrl := gomock.NewController(t) @@ -631,3 +632,28 @@ func TestHandleRemoveFailed(t *testing.T) { t.Fatalf("expected HTTP status code %v got %v", http.StatusInternalServerError, rr.Status) } } + +func TestRegisterValidation(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockLogger := NewMockLoggerInterface(ctrl) + mockService := NewMockServiceInterface(ctrl) + mockValidationRegistry := NewMockValidationRegistryInterface(ctrl) + + apiKey := "rules" + mockValidationRegistry.EXPECT(). + RegisterPayloadValidator(gomock.Eq(apiKey), gomock.Any()). + Return(nil) + mockValidationRegistry.EXPECT(). + RegisterPayloadValidator(gomock.Eq(apiKey), gomock.Any()). + Return(fmt.Errorf("key is already registered")) + + // first registration of `apiKey` is successful + NewAPI(mockService, mockLogger).RegisterValidation(mockValidationRegistry) + + mockLogger.EXPECT().Fatalf(gomock.Any(), gomock.Any()).Times(1) + + // second registration of `apiKey` causes logger.Fatal invocation + NewAPI(mockService, mockLogger).RegisterValidation(mockValidationRegistry) +} diff --git a/pkg/rules/validation_test.go b/pkg/rules/validation_test.go new file mode 100644 index 000000000..88e9d43e0 --- /dev/null +++ b/pkg/rules/validation_test.go @@ -0,0 +1,249 @@ +// Copyright 2024 Canonical Ltd. +// SPDX-License-Identifier: AGPL-3.0 + +package rules + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-playground/validator/v10" + oathkeeper "github.com/ory/oathkeeper-client-go" + + "github.com/canonical/identity-platform-admin-ui/internal/validation" +) + +func TestNeedsValidation(t *testing.T) { + p := new(PayloadValidator) + p.validator = validation.NewValidator() + p.setupValidator() + + for _, tt := range []struct { + name string + req *http.Request + expectedResult bool + }{ + { + name: http.MethodPost, + req: httptest.NewRequest(http.MethodPost, "/", nil), + expectedResult: true, + }, + { + name: http.MethodPut, + req: httptest.NewRequest(http.MethodPut, "/", nil), + expectedResult: true, + }, + { + name: http.MethodGet, + req: httptest.NewRequest(http.MethodGet, "/", nil), + expectedResult: false, + }, + { + name: http.MethodPatch, + req: httptest.NewRequest(http.MethodPatch, "/", nil), + expectedResult: false, + }, + { + name: http.MethodDelete, + req: httptest.NewRequest(http.MethodDelete, "/", nil), + expectedResult: false, + }, + { + name: http.MethodConnect, + req: httptest.NewRequest(http.MethodConnect, "/", nil), + expectedResult: false, + }, + { + name: http.MethodHead, + req: httptest.NewRequest(http.MethodHead, "/", nil), + expectedResult: false, + }, + { + name: http.MethodTrace, + req: httptest.NewRequest(http.MethodTrace, "/", nil), + expectedResult: false, + }, + { + name: http.MethodOptions, + req: httptest.NewRequest(http.MethodOptions, "/", nil), + expectedResult: false, + }, + } { + tt := tt + t.Run(tt.name, func(t *testing.T) { + result := p.NeedsValidation(tt.req) + + if result != tt.expectedResult { + t.Fatalf("Result doesn't match expected one, obtained %t instead of %t", result, tt.expectedResult) + } + }) + } + +} + +func TestValidate(t *testing.T) { + p := new(PayloadValidator) + p.apiKey = "rules" + p.validator = validation.NewValidator() + p.setupValidator() + + mockHandlerIdentifier := "mock-handler" + mockUrl := "mock-url-with-oathkeeper-rules" + + for _, tt := range []struct { + name string + method string + endpoint string + body func() []byte + expectedResult validator.ValidationErrors + expectedError error + }{ + { + name: "CreateRuleSuccess", + method: http.MethodPost, + endpoint: "", + body: func() []byte { + rule := new(oathkeeper.Rule) + rule.Authorizer = &oathkeeper.RuleHandler{ + Handler: &mockHandlerIdentifier, + } + rule.Authenticators = []oathkeeper.RuleHandler{ + { + Handler: &mockHandlerIdentifier, + }, + } + + rule.Match = &oathkeeper.RuleMatch{ + Methods: []string{"GET", "POST"}, + Url: &mockUrl, + } + + marshal, _ := json.Marshal(rule) + return marshal + }, + expectedResult: nil, + expectedError: nil, + }, + { + name: "UpdateRuleSuccess", + method: http.MethodPut, + endpoint: "/", + body: func() []byte { + rule := new(oathkeeper.Rule) + rule.Authorizer = &oathkeeper.RuleHandler{ + Handler: &mockHandlerIdentifier, + } + rule.Authenticators = []oathkeeper.RuleHandler{ + { + Handler: &mockHandlerIdentifier, + }, + } + + rule.Match = &oathkeeper.RuleMatch{ + Methods: []string{"GET", "POST"}, + Url: &mockUrl, + } + + marshal, _ := json.Marshal(rule) + return marshal + }, + expectedResult: nil, + expectedError: nil, + }, + { + name: "NoMatch", + method: http.MethodPost, + endpoint: "no-match-endpoint", + body: func() []byte { + return nil + }, + expectedResult: nil, + expectedError: validation.NoMatchError(p.apiKey), + }, + { + name: "CreateRuleValidationError", + method: http.MethodPost, + endpoint: "", + body: func() []byte { + rule := new(oathkeeper.Rule) + rule.Authorizer = &oathkeeper.RuleHandler{ + Handler: &mockHandlerIdentifier, + } + rule.Authenticators = []oathkeeper.RuleHandler{ + { + Handler: &mockHandlerIdentifier, + }, + } + + rule.Match = &oathkeeper.RuleMatch{ + Methods: []string{}, + Url: &mockUrl, + } + + marshal, _ := json.Marshal(rule) + return marshal + }, + expectedResult: validator.ValidationErrors{}, + expectedError: nil, + }, + { + name: "UpdateRuleValidationError", + method: http.MethodPut, + endpoint: "/identity-id", + body: func() []byte { + wrongHandlerIdentifier := "" + + rule := new(oathkeeper.Rule) + rule.Authorizer = &oathkeeper.RuleHandler{} + rule.Authenticators = []oathkeeper.RuleHandler{ + { + Handler: &wrongHandlerIdentifier, + }, + } + + rule.Match = &oathkeeper.RuleMatch{ + Methods: []string{"GET", "non-http-verb"}, + Url: &mockUrl, + } + + marshal, _ := json.Marshal(rule) + return marshal + }, + expectedResult: validator.ValidationErrors{}, + expectedError: nil, + }, + } { + tt := tt + t.Run(tt.name, func(t *testing.T) { + _, result, err := p.Validate(context.TODO(), tt.method, tt.endpoint, tt.body()) + + if err != nil { + if tt.expectedError == nil { + t.Fatalf("Unexpected error for '%s'", tt.name) + } + + if err.Error() != tt.expectedError.Error() { + t.Fatalf("Returned error doesn't match expected, obtained '%v' instead of '%v'", err, tt.expectedError) + } + + return + } + + if result != nil { + if tt.expectedResult == nil { + t.Fatalf("Unexpected result for '%s'", tt.name) + } + + if errors.Is(result, tt.expectedResult) { + t.Fatalf("Returned validation errors don't match expected, obtained '%v' instead of '%v'", result, tt.expectedResult) + } + + return + } + }) + } +} From 6bf72e5d75d94daca1fdf028ed1a3f7744e67b4b Mon Sep 17 00:00:00 2001 From: barco Date: Thu, 18 Apr 2024 16:04:38 +0200 Subject: [PATCH 09/17] feat(roles): add validation implementation --- pkg/roles/handlers.go | 4 +- pkg/roles/validation.go | 84 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 2 deletions(-) create mode 100644 pkg/roles/validation.go diff --git a/pkg/roles/handlers.go b/pkg/roles/handlers.go index 7065b1cc1..efdfd4315 100644 --- a/pkg/roles/handlers.go +++ b/pkg/roles/handlers.go @@ -30,7 +30,7 @@ type Permission struct { } type UpdatePermissionsRequest struct { - Permissions []Permission `json:"permissions" validate:"required"` + Permissions []Permission `json:"permissions" validate:"required,dive,required"` } type RoleRequest struct { @@ -467,7 +467,7 @@ func NewAPI(service ServiceInterface, tracer tracing.TracingInterface, monitor m a := new(API) a.service = service - //a.payloadValidator = NewRolesPayloadValidator(a.apiKey) + a.payloadValidator = NewRolesPayloadValidator(a.apiKey) a.logger = logger a.tracer = tracer a.monitor = monitor diff --git a/pkg/roles/validation.go b/pkg/roles/validation.go new file mode 100644 index 000000000..84e2e785a --- /dev/null +++ b/pkg/roles/validation.go @@ -0,0 +1,84 @@ +// Copyright 2024 Canonical Ltd. +// SPDX-License-Identifier: AGPL-3.0 + +package roles + +import ( + "context" + "encoding/json" + "net/http" + "strings" + + "github.com/go-playground/validator/v10" + + "github.com/canonical/identity-platform-admin-ui/internal/validation" +) + +type PayloadValidator struct { + apiKey string + validator *validator.Validate +} + +func (p *PayloadValidator) NeedsValidation(r *http.Request) bool { + return r.Method == http.MethodPost || r.Method == http.MethodPatch +} + +func (p *PayloadValidator) Validate(ctx context.Context, method, endpoint string, body []byte) (context.Context, validator.ValidationErrors, error) { + validated := false + var err error + + if p.isCreateRole(method, endpoint) { + roleRequest := new(RoleRequest) + if err := json.Unmarshal(body, roleRequest); err != nil { + return ctx, nil, err + } + + err = p.validator.Struct(roleRequest) + validated = true + } + + if p.isUpdateRole(method, endpoint) { + // TODO: @barco to implement when the UpdateGroup is implemented + validated = true + } + + if p.isAssignPermissions(method, endpoint) { + updatePermissions := new(UpdatePermissionsRequest) + if err := json.Unmarshal(body, updatePermissions); err != nil { + return ctx, nil, err + } + + err = p.validator.Struct(updatePermissions) + validated = true + } + + if !validated { + return ctx, nil, validation.NoMatchError(p.apiKey) + } + + if err == nil { + return ctx, nil, nil + } + + return ctx, err.(validator.ValidationErrors), nil +} + +func (p *PayloadValidator) isCreateRole(method, endpoint string) bool { + return method == http.MethodPost && endpoint == "" +} + +func (p *PayloadValidator) isUpdateRole(method, endpoint string) bool { + return method == http.MethodPatch && strings.HasPrefix(endpoint, "/") +} + +func (p *PayloadValidator) isAssignPermissions(method, endpoint string) bool { + return method == http.MethodPatch && strings.HasSuffix(endpoint, "/entitlements") +} + +func NewRolesPayloadValidator(apiKey string) *PayloadValidator { + p := new(PayloadValidator) + p.apiKey = apiKey + p.validator = validation.NewValidator() + + return p +} From 2011a88bcc51a951c35bf7710122a7fe452ddaa7 Mon Sep 17 00:00:00 2001 From: barco Date: Thu, 18 Apr 2024 16:05:01 +0200 Subject: [PATCH 10/17] test(roles): validation --- pkg/roles/handlers_test.go | 28 ++++ pkg/roles/validation_test.go | 245 +++++++++++++++++++++++++++++++++++ 2 files changed, 273 insertions(+) create mode 100644 pkg/roles/validation_test.go diff --git a/pkg/roles/handlers_test.go b/pkg/roles/handlers_test.go index ca5b89fa8..6cda54195 100644 --- a/pkg/roles/handlers_test.go +++ b/pkg/roles/handlers_test.go @@ -29,6 +29,7 @@ import ( //go:generate mockgen -build_flags=--mod=mod -package roles -destination ./mock_interfaces.go -source=./interfaces.go //go:generate mockgen -build_flags=--mod=mod -package roles -destination ./mock_monitor.go -source=../../internal/monitoring/interfaces.go //go:generate mockgen -build_flags=--mod=mod -package roles -destination ./mock_tracing.go go.opentelemetry.io/otel/trace Tracer +//go:generate mockgen -build_flags=--mod=mod -package roles -destination ./mock_validation.go -source=../../internal/validation/registry.go // + http :8000/api/v0/roles X-Authorization:c2hpcHBlcml6ZXI= // HTTP/1.1 200 OK @@ -1327,3 +1328,30 @@ func TestHandleCreateBadRoleFormat(t *testing.T) { }) } } + +func TestRegisterValidation(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockLogger := NewMockLoggerInterface(ctrl) + mockTracer := NewMockTracer(ctrl) + mockMonitor := monitoring.NewMockMonitorInterface(ctrl) + mockService := NewMockServiceInterface(ctrl) + mockValidationRegistry := NewMockValidationRegistryInterface(ctrl) + + apiKey := "roles" + mockValidationRegistry.EXPECT(). + RegisterPayloadValidator(gomock.Eq(apiKey), gomock.Any()). + Return(nil) + mockValidationRegistry.EXPECT(). + RegisterPayloadValidator(gomock.Eq(apiKey), gomock.Any()). + Return(fmt.Errorf("key is already registered")) + + // first registration of `apiKey` is successful + NewAPI(mockService, mockTracer, mockMonitor, mockLogger).RegisterValidation(mockValidationRegistry) + + mockLogger.EXPECT().Fatalf(gomock.Any(), gomock.Any()).Times(1) + + // second registration of `apiKey` causes logger.Fatal invocation + NewAPI(mockService, mockTracer, mockMonitor, mockLogger).RegisterValidation(mockValidationRegistry) +} diff --git a/pkg/roles/validation_test.go b/pkg/roles/validation_test.go new file mode 100644 index 000000000..95b1b25d9 --- /dev/null +++ b/pkg/roles/validation_test.go @@ -0,0 +1,245 @@ +// Copyright 2024 Canonical Ltd. +// SPDX-License-Identifier: AGPL-3.0 + +package roles + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-playground/validator/v10" + + "github.com/canonical/identity-platform-admin-ui/internal/validation" +) + +func TestNeedsValidation(t *testing.T) { + p := new(PayloadValidator) + p.validator = validation.NewValidator() + + for _, tt := range []struct { + name string + req *http.Request + expectedResult bool + }{ + { + name: http.MethodPost, + req: httptest.NewRequest(http.MethodPost, "/", nil), + expectedResult: true, + }, + { + name: http.MethodPatch, + req: httptest.NewRequest(http.MethodPatch, "/", nil), + expectedResult: true, + }, + { + name: http.MethodPut, + req: httptest.NewRequest(http.MethodPut, "/", nil), + expectedResult: false, + }, + { + name: http.MethodGet, + req: httptest.NewRequest(http.MethodGet, "/", nil), + expectedResult: false, + }, + { + name: http.MethodDelete, + req: httptest.NewRequest(http.MethodDelete, "/", nil), + expectedResult: false, + }, + { + name: http.MethodConnect, + req: httptest.NewRequest(http.MethodConnect, "/", nil), + expectedResult: false, + }, + { + name: http.MethodHead, + req: httptest.NewRequest(http.MethodHead, "/", nil), + expectedResult: false, + }, + { + name: http.MethodTrace, + req: httptest.NewRequest(http.MethodTrace, "/", nil), + expectedResult: false, + }, + { + name: http.MethodOptions, + req: httptest.NewRequest(http.MethodOptions, "/", nil), + expectedResult: false, + }, + } { + tt := tt + t.Run(tt.name, func(t *testing.T) { + result := p.NeedsValidation(tt.req) + + if result != tt.expectedResult { + t.Fatalf("Result doesn't match expected one, obtained %t instead of %t", result, tt.expectedResult) + } + }) + } + +} + +func TestValidate(t *testing.T) { + p := new(PayloadValidator) + p.apiKey = "roles" + p.validator = validation.NewValidator() + + for _, tt := range []struct { + name string + method string + endpoint string + body func() []byte + expectedResult validator.ValidationErrors + expectedError error + }{ + { + name: "CreateRoleSuccess", + method: http.MethodPost, + endpoint: "", + body: func() []byte { + role := new(RoleRequest) + role.ID = "mock-role-id" + + marshal, _ := json.Marshal(role) + return marshal + }, + expectedResult: nil, + expectedError: nil, + }, + { + name: "UpdateRoleSuccess", + method: http.MethodPatch, + endpoint: "/", + body: func() []byte { + role := new(RoleRequest) + role.ID = "mock-role-id" + + marshal, _ := json.Marshal(role) + return marshal + }, + expectedResult: nil, + expectedError: nil, + }, + { + name: "AssignPermissionsSuccess", + method: http.MethodPatch, + endpoint: "/", + body: func() []byte { + permissionsRequest := new(UpdatePermissionsRequest) + permissionsRequest.Permissions = []Permission{ + { + Relation: "mock-relation", + Object: "mock-object", + }, + } + + marshal, _ := json.Marshal(permissionsRequest) + return marshal + }, + expectedResult: nil, + expectedError: nil, + }, + { + name: "AssignPermissionsEmptySuccess", + method: http.MethodPatch, + endpoint: "/", + body: func() []byte { + permissionsRequest := new(UpdatePermissionsRequest) + permissionsRequest.Permissions = []Permission{} + + marshal, _ := json.Marshal(permissionsRequest) + return marshal + }, + expectedResult: nil, + expectedError: nil, + }, + { + name: "NoMatch", + method: http.MethodPost, + endpoint: "no-match-endpoint", + body: func() []byte { + return nil + }, + expectedResult: nil, + expectedError: validation.NoMatchError(p.apiKey), + }, + { + name: "CreateRoleValidationError", + method: http.MethodPost, + endpoint: "", + body: func() []byte { + role := new(RoleRequest) + + marshal, _ := json.Marshal(role) + return marshal + }, + expectedResult: validator.ValidationErrors{}, + expectedError: nil, + }, + { + name: "UpdateRoleValidationError", + method: http.MethodPatch, + endpoint: "/identity-id", + body: func() []byte { + role := new(RoleRequest) + + marshal, _ := json.Marshal(role) + return marshal + }, + expectedResult: validator.ValidationErrors{}, + expectedError: nil, + }, + { + name: "AssignPermissionsValidationError", + method: http.MethodPatch, + endpoint: "/identity-id", + body: func() []byte { + permissionsRequest := new(UpdatePermissionsRequest) + permissionsRequest.Permissions = []Permission{ + { + Relation: "mock-relation", + Object: "", + }, + } + + marshal, _ := json.Marshal(permissionsRequest) + return marshal + }, + expectedResult: validator.ValidationErrors{}, + expectedError: nil, + }, + } { + tt := tt + t.Run(tt.name, func(t *testing.T) { + _, result, err := p.Validate(context.TODO(), tt.method, tt.endpoint, tt.body()) + + if err != nil { + if tt.expectedError == nil { + t.Fatalf("Unexpected error for '%s'", tt.name) + } + + if err.Error() != tt.expectedError.Error() { + t.Fatalf("Returned error doesn't match expected, obtained '%v' instead of '%v'", err, tt.expectedError) + } + + return + } + + if result != nil { + if tt.expectedResult == nil { + t.Fatalf("Unexpected result for '%s'", tt.name) + } + + if errors.Is(result, tt.expectedResult) { + t.Fatalf("Returned validation errors don't match expected, obtained '%v' instead of '%v'", result, tt.expectedResult) + } + + return + } + }) + } +} From 71ff6612485dd73374e09508143d50f455a46270 Mon Sep 17 00:00:00 2001 From: barco Date: Thu, 18 Apr 2024 16:06:10 +0200 Subject: [PATCH 11/17] feat(idp): add validation implementation --- pkg/idp/handlers.go | 2 +- pkg/idp/third_party.go | 24 +++++++------- pkg/idp/validation.go | 71 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 13 deletions(-) create mode 100644 pkg/idp/validation.go diff --git a/pkg/idp/handlers.go b/pkg/idp/handlers.go index ca49c88eb..bfdce22e7 100644 --- a/pkg/idp/handlers.go +++ b/pkg/idp/handlers.go @@ -256,7 +256,7 @@ func NewAPI(service ServiceInterface, logger logging.LoggerInterface) *API { a := new(API) a.apiKey = "idps" - //a.payloadValidator = NewIdPPayloadValidator(a.apiKey) + a.payloadValidator = NewIdPPayloadValidator(a.apiKey) a.service = service a.logger = logger diff --git a/pkg/idp/third_party.go b/pkg/idp/third_party.go index e8e1b6c8f..569d1f914 100644 --- a/pkg/idp/third_party.go +++ b/pkg/idp/third_party.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical Ltd +// Copyright 2024 Canonical Ltd. // SPDX-License-Identifier: AGPL-3.0 package idp @@ -31,7 +31,7 @@ type Configuration struct { // - dingtalk // - linkedin // - patreon - Provider string `json:"provider" yaml:"provider"` + Provider string `json:"provider" yaml:"provider" validate:"required,supported_provider"` // Label represents an optional label which can be used in the UI generation. Label string `json:"label"` @@ -44,52 +44,52 @@ type Configuration struct { // IssuerURL is the OpenID Connect Server URL. You can leave this empty if `provider` is not set to `generic`. // If set, neither `auth_url` nor `token_url` are required. - IssuerURL string `json:"issuer_url" yaml:"issuer_url"` + IssuerURL string `json:"issuer_url" yaml:"issuer_url" validate:"required_if=Provider generic"` // AuthURL is the authorize url, typically something like: https://example.org/oauth2/auth // Should only be used when the OAuth2 / OpenID Connect server is not supporting OpenID Connect Discovery and when // `provider` is set to `generic`. - AuthURL string `json:"auth_url" yaml:"auth_url"` + AuthURL string `json:"auth_url" yaml:"auth_url" validate:"required_if=Provider generic"` // TokenURL is the token url, typically something like: https://example.org/oauth2/token // Should only be used when the OAuth2 / OpenID Connect server is not supporting OpenID Connect Discovery and when // `provider` is set to `generic`. - TokenURL string `json:"token_url" yaml:"token_url"` + TokenURL string `json:"token_url" yaml:"token_url" validate:"required_if=Provider generic"` // Tenant is the Azure AD Tenant to use for authentication, and must be set when `provider` is set to `microsoft`. // Can be either `common`, `organizations`, `consumers` for a multitenant application or a specific tenant like // `8eaef023-2b34-4da1-9baa-8bc8c9d6a490` or `contoso.onmicrosoft.com`. - Tenant string `json:"microsoft_tenant" yaml:"microsoft_tenant"` + Tenant string `json:"microsoft_tenant" yaml:"microsoft_tenant" validate:"required_if=Provider microsoft"` // SubjectSource is a flag which controls from which endpoint the subject identifier is taken by microsoft provider. // Can be either `userinfo` or `me`. // If the value is `userinfo` then the subject identifier is taken from sub field of userinfo standard endpoint response. // If the value is `me` then the `id` field of https://graph.microsoft.com/v1.0/me response is taken as subject. // The default is `userinfo`. - SubjectSource string `json:"subject_source" yaml:"subject_source"` + SubjectSource string `json:"subject_source" yaml:"subject_source" validate:"oneof=userinfo me"` // TeamId is the Apple Developer Team ID that's needed for the `apple` `provider` to work. // It can be found Apple Developer website and combined with `apple_private_key` and `apple_private_key_id` // is used to generate `client_secret` - TeamId string `json:"apple_team_id" yaml:"apple_team_id"` + TeamId string `json:"apple_team_id" yaml:"apple_team_id" validate:"required_if=Provider apple"` // PrivateKeyId is the private Apple key identifier. Keys can be generated via developer.apple.com. // This key should be generated with the `Sign In with Apple` option checked. // This is needed when `provider` is set to `apple` - PrivateKeyId string `json:"apple_private_key_id" yaml:"apple_private_key_id"` + PrivateKeyId string `json:"apple_private_key_id" yaml:"apple_private_key_id" validate:"required_if=Provider apple"` // PrivateKeyId is the Apple private key identifier that can be downloaded during key generation. // This is needed when `provider` is set to `apple` - PrivateKey string `json:"apple_private_key" yaml:"apple_private_key"` + PrivateKey string `json:"apple_private_key" yaml:"apple_private_key" validate:"required_if=Provider apple"` // Scope specifies optional requested permissions. - Scope []string `json:"scope" yaml:"scope"` + Scope []string `json:"scope" yaml:"scope" validate:"omitempty,dive,required"` // Mapper specifies the JSONNet code snippet which uses the OpenID Connect Provider's data (e.g. GitHub or Google // profile information) to hydrate the identity's data. // // It can be either a URL (file://, http(s)://, base64://) or an inline JSONNet code snippet. - Mapper string `json:"mapper_url" yaml:"mapper_url"` + Mapper string `json:"mapper_url" yaml:"mapper_url" validate:"required"` // RequestedClaims string encoded json object that specifies claims and optionally their properties which should be // included in the id_token or returned from the UserInfo Endpoint. diff --git a/pkg/idp/validation.go b/pkg/idp/validation.go new file mode 100644 index 000000000..2ae4f0b97 --- /dev/null +++ b/pkg/idp/validation.go @@ -0,0 +1,71 @@ +// Copyright 2024 Canonical Ltd. +// SPDX-License-Identifier: AGPL-3.0 + +package idp + +import ( + "context" + "encoding/json" + "net/http" + "strings" + + "github.com/go-playground/validator/v10" + + "github.com/canonical/identity-platform-admin-ui/internal/validation" +) + +type PayloadValidator struct { + apiKey string + validator *validator.Validate +} + +func (p *PayloadValidator) setupValidator() { + p.validator.RegisterAlias("supported_provider", "oneof=generic google github githubapp gitlab microsoft discord slack facebook auth0 vk yandex apple spotify netid dingtalk linkedin patreon") +} + +func (p *PayloadValidator) NeedsValidation(r *http.Request) bool { + return r.Method == http.MethodPost || r.Method == http.MethodPatch +} + +func (p *PayloadValidator) Validate(ctx context.Context, method, endpoint string, body []byte) (context.Context, validator.ValidationErrors, error) { + validated := false + var err error + + if p.isCreateIdP(method, endpoint) || p.isPartialUpdateIdP(method, endpoint) { + conf := new(Configuration) + if err := json.Unmarshal(body, conf); err != nil { + return ctx, nil, err + } + + err = p.validator.Struct(conf) + validated = true + } + + if !validated { + return ctx, nil, validation.NoMatchError(p.apiKey) + } + + if err == nil { + return ctx, nil, nil + } + + return ctx, err.(validator.ValidationErrors), nil +} + +func (p *PayloadValidator) isCreateIdP(method, endpoint string) bool { + return method == http.MethodPost && endpoint == "" +} + +func (p *PayloadValidator) isPartialUpdateIdP(method, endpoint string) bool { + return method == http.MethodPatch && strings.HasPrefix(endpoint, "/") +} + +func NewIdPPayloadValidator(apiKey string) *PayloadValidator { + p := new(PayloadValidator) + p.apiKey = apiKey + p.validator = validation.NewValidator() + + p.setupValidator() + + return p +} From e6bd1cf437e1a478be1685082289c39157d8bfe9 Mon Sep 17 00:00:00 2001 From: barco Date: Thu, 18 Apr 2024 16:06:34 +0200 Subject: [PATCH 12/17] test(idp): validation --- pkg/idp/handlers_test.go | 28 ++++- pkg/idp/validation_test.go | 213 +++++++++++++++++++++++++++++++++++++ 2 files changed, 240 insertions(+), 1 deletion(-) create mode 100644 pkg/idp/validation_test.go diff --git a/pkg/idp/handlers_test.go b/pkg/idp/handlers_test.go index 99a81d0ef..eaff46b6e 100644 --- a/pkg/idp/handlers_test.go +++ b/pkg/idp/handlers_test.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical Ltd +// Copyright 2024 Canonical Ltd. // SPDX-License-Identifier: AGPL-3.0 package idp @@ -27,6 +27,7 @@ import ( //go:generate mockgen -build_flags=--mod=mod -package idp -destination ./mock_monitor.go -source=../../internal/monitoring/interfaces.go //go:generate mockgen -build_flags=--mod=mod -package idp -destination ./mock_tracing.go go.opentelemetry.io/otel/trace Tracer //go:generate mockgen -build_flags=--mod=mod -package idp -destination ./mock_corev1.go k8s.io/client-go/kubernetes/typed/core/v1 CoreV1Interface,ConfigMapInterface +//go:generate mockgen -build_flags=--mod=mod -package idp -destination ./mock_validation.go -source=../../internal/validation/registry.go func TestHandleListSuccess(t *testing.T) { ctrl := gomock.NewController(t) @@ -782,3 +783,28 @@ func TestHandleRemoveFails(t *testing.T) { t.Errorf("expected code to be %v got %v", http.StatusInternalServerError, rr.Status) } } + +func TestRegisterValidation(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockLogger := NewMockLoggerInterface(ctrl) + mockService := NewMockServiceInterface(ctrl) + mockValidationRegistry := NewMockValidationRegistryInterface(ctrl) + + apiKey := "idps" + mockValidationRegistry.EXPECT(). + RegisterPayloadValidator(gomock.Eq(apiKey), gomock.Any()). + Return(nil) + mockValidationRegistry.EXPECT(). + RegisterPayloadValidator(gomock.Eq(apiKey), gomock.Any()). + Return(fmt.Errorf("key is already registered")) + + // first registration of `apiKey` is successful + NewAPI(mockService, mockLogger).RegisterValidation(mockValidationRegistry) + + mockLogger.EXPECT().Fatalf(gomock.Any(), gomock.Any()).Times(1) + + // second registration of `apiKey` causes logger.Fatal invocation + NewAPI(mockService, mockLogger).RegisterValidation(mockValidationRegistry) +} diff --git a/pkg/idp/validation_test.go b/pkg/idp/validation_test.go new file mode 100644 index 000000000..87293735e --- /dev/null +++ b/pkg/idp/validation_test.go @@ -0,0 +1,213 @@ +// Copyright 2024 Canonical Ltd. +// SPDX-License-Identifier: AGPL-3.0 + +package idp + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-playground/validator/v10" + + "github.com/canonical/identity-platform-admin-ui/internal/validation" +) + +func TestNeedsValidation(t *testing.T) { + p := new(PayloadValidator) + p.validator = validation.NewValidator() + + for _, tt := range []struct { + name string + req *http.Request + expectedResult bool + }{ + { + name: http.MethodPost, + req: httptest.NewRequest(http.MethodPost, "/", nil), + expectedResult: true, + }, + { + name: http.MethodPatch, + req: httptest.NewRequest(http.MethodPatch, "/", nil), + expectedResult: true, + }, + { + name: http.MethodPut, + req: httptest.NewRequest(http.MethodPut, "/", nil), + expectedResult: false, + }, + { + name: http.MethodGet, + req: httptest.NewRequest(http.MethodGet, "/", nil), + expectedResult: false, + }, + { + name: http.MethodDelete, + req: httptest.NewRequest(http.MethodDelete, "/", nil), + expectedResult: false, + }, + { + name: http.MethodConnect, + req: httptest.NewRequest(http.MethodConnect, "/", nil), + expectedResult: false, + }, + { + name: http.MethodHead, + req: httptest.NewRequest(http.MethodHead, "/", nil), + expectedResult: false, + }, + { + name: http.MethodTrace, + req: httptest.NewRequest(http.MethodTrace, "/", nil), + expectedResult: false, + }, + { + name: http.MethodOptions, + req: httptest.NewRequest(http.MethodOptions, "/", nil), + expectedResult: false, + }, + } { + tt := tt + t.Run(tt.name, func(t *testing.T) { + result := p.NeedsValidation(tt.req) + + if result != tt.expectedResult { + t.Fatalf("Result doesn't match expected one, obtained %t instead of %t", result, tt.expectedResult) + } + }) + } + +} + +func TestValidate(t *testing.T) { + p := new(PayloadValidator) + p.apiKey = "roles" + p.validator = validation.NewValidator() + + p.setupValidator() + + for _, tt := range []struct { + name string + method string + endpoint string + body func() []byte + expectedResult validator.ValidationErrors + expectedError error + }{ + { + name: "CreateIdPSuccess", + method: http.MethodPost, + endpoint: "", + body: func() []byte { + conf := new(Configuration) + conf.Provider = "generic" + conf.IssuerURL = "mock-url" + conf.AuthURL = "mock-url" + conf.TokenURL = "mock-url" + conf.SubjectSource = "me" + conf.Scope = []string{} + conf.Mapper = "mock-url" + + marshal, _ := json.Marshal(conf) + return marshal + }, + expectedResult: nil, + expectedError: nil, + }, + { + name: "PartialUpdateIdPSuccess", + method: http.MethodPatch, + endpoint: "/", + body: func() []byte { + conf := new(Configuration) + conf.Provider = "microsoft" + conf.Tenant = "mock-tenant" + conf.SubjectSource = "me" + conf.Scope = []string{"profile"} + conf.Mapper = "mock-url" + + marshal, _ := json.Marshal(conf) + return marshal + }, + expectedResult: nil, + expectedError: nil, + }, + { + name: "NoMatch", + method: http.MethodPost, + endpoint: "no-match-endpoint", + body: func() []byte { + return nil + }, + expectedResult: nil, + expectedError: validation.NoMatchError(p.apiKey), + }, + { + name: "CreateIdPValidationError", + method: http.MethodPost, + endpoint: "", + body: func() []byte { + conf := new(Configuration) + conf.Provider = "microsoft" + conf.SubjectSource = "me" + conf.Scope = []string{"profile"} + conf.Mapper = "mock-url" + + marshal, _ := json.Marshal(conf) + return marshal + }, + expectedResult: validator.ValidationErrors{}, + expectedError: nil, + }, + { + name: "PartialUpdateIdPValidationError", + method: http.MethodPatch, + endpoint: "/identity-id", + body: func() []byte { + conf := new(Configuration) + conf.Provider = "apple" + conf.SubjectSource = "me" + conf.Scope = []string{""} + conf.Mapper = "mock-url" + + marshal, _ := json.Marshal(conf) + return marshal + }, + expectedResult: validator.ValidationErrors{}, + expectedError: nil, + }, + } { + tt := tt + t.Run(tt.name, func(t *testing.T) { + _, result, err := p.Validate(context.TODO(), tt.method, tt.endpoint, tt.body()) + + if err != nil { + if tt.expectedError == nil { + t.Fatalf("Unexpected error for '%s'", tt.name) + } + + if err.Error() != tt.expectedError.Error() { + t.Fatalf("Returned error doesn't match expected, obtained '%v' instead of '%v'", err, tt.expectedError) + } + + return + } + + if result != nil { + if tt.expectedResult == nil { + t.Fatalf("Unexpected result for '%s'", tt.name) + } + + if errors.Is(result, tt.expectedResult) { + t.Fatalf("Returned validation errors don't match expected, obtained '%v' instead of '%v'", result, tt.expectedResult) + } + + return + } + }) + } +} From b4fa7629306681e25b16c9d7cadcfdcd96fdef02 Mon Sep 17 00:00:00 2001 From: barco Date: Thu, 18 Apr 2024 16:07:19 +0200 Subject: [PATCH 13/17] fix(identities): validation and improved tests --- pkg/identities/validation_test.go | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/pkg/identities/validation_test.go b/pkg/identities/validation_test.go index 96b20712a..adcb70f19 100644 --- a/pkg/identities/validation_test.go +++ b/pkg/identities/validation_test.go @@ -206,11 +206,27 @@ func TestValidate(t *testing.T) { t.Run(tt.name, func(t *testing.T) { _, result, err := p.Validate(context.TODO(), tt.method, tt.endpoint, tt.body()) - if err != nil && err.Error() != tt.expectedError.Error() { - t.Fatalf("Returned error doesn't match expected, obtained '%v' instead of '%v'", err, tt.expectedError) - } else if result != nil && errors.Is(result, tt.expectedResult) { - t.Fatalf("Returned validation errors don't match expected, obtained '%v' instead of '%v'", result, tt.expectedResult) - } else { + if err != nil { + if tt.expectedError == nil { + t.Fatalf("Unexpected error for '%s'", tt.name) + } + + if err.Error() != tt.expectedError.Error() { + t.Fatalf("Returned error doesn't match expected, obtained '%v' instead of '%v'", err, tt.expectedError) + } + + return + } + + if result != nil { + if tt.expectedResult == nil { + t.Fatalf("Unexpected result for '%s'", tt.name) + } + + if errors.Is(result, tt.expectedResult) { + t.Fatalf("Returned validation errors don't match expected, obtained '%v' instead of '%v'", result, tt.expectedResult) + } + return } }) From 255733e3d5499181c2ef9b92f9145ae7997541ce Mon Sep 17 00:00:00 2001 From: barco Date: Thu, 18 Apr 2024 16:07:27 +0200 Subject: [PATCH 14/17] fix(groups): validation and improved tests --- pkg/groups/validation_test.go | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/pkg/groups/validation_test.go b/pkg/groups/validation_test.go index d1bfa4053..9a2f498f9 100644 --- a/pkg/groups/validation_test.go +++ b/pkg/groups/validation_test.go @@ -267,11 +267,27 @@ func TestValidate(t *testing.T) { t.Run(tt.name, func(t *testing.T) { _, result, err := p.Validate(context.TODO(), tt.method, tt.endpoint, tt.body()) - if err != nil && err.Error() != tt.expectedError.Error() { - t.Fatalf("Returned error doesn't match expected, obtained '%v' instead of '%v'", err, tt.expectedError) - } else if result != nil && errors.Is(result, tt.expectedResult) { - t.Fatalf("Returned validation errors don't match expected, obtained '%v' instead of '%v'", result, tt.expectedResult) - } else { + if err != nil { + if tt.expectedError == nil { + t.Fatalf("Unexpected error for '%s'", tt.name) + } + + if err.Error() != tt.expectedError.Error() { + t.Fatalf("Returned error doesn't match expected, obtained '%v' instead of '%v'", err, tt.expectedError) + } + + return + } + + if result != nil { + if tt.expectedResult == nil { + t.Fatalf("Unexpected result for '%s'", tt.name) + } + + if errors.Is(result, tt.expectedResult) { + t.Fatalf("Returned validation errors don't match expected, obtained '%v' instead of '%v'", result, tt.expectedResult) + } + return } }) From 129a8a8b40ae33cf2f531fae721c17837f12cb7e Mon Sep 17 00:00:00 2001 From: barco Date: Thu, 18 Apr 2024 16:07:34 +0200 Subject: [PATCH 15/17] fix(clients): validation and improved tests --- pkg/clients/handlers.go | 2 +- pkg/clients/handlers_test.go | 2 +- pkg/clients/validation_test.go | 26 +++++++++++++++++++++----- 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/pkg/clients/handlers.go b/pkg/clients/handlers.go index 0c0691c56..ba56fce4a 100644 --- a/pkg/clients/handlers.go +++ b/pkg/clients/handlers.go @@ -35,7 +35,7 @@ func (a *API) RegisterEndpoints(mux *chi.Mux) { func (a *API) RegisterValidation(v validation.ValidationRegistryInterface) { err := v.RegisterPayloadValidator(a.apiKey, a.payloadValidator) if err != nil { - a.logger.Fatalf("unexpected error while registering PayloadValidator for 'clients', %s", err) + a.logger.Fatalf("unexpected error while registering PayloadValidator for clients, %s", err) } } diff --git a/pkg/clients/handlers_test.go b/pkg/clients/handlers_test.go index 605272123..246a7636c 100644 --- a/pkg/clients/handlers_test.go +++ b/pkg/clients/handlers_test.go @@ -594,7 +594,7 @@ func TestRegisterValidation(t *testing.T) { // first registration of `apiKey` is successful NewAPI(mockService, mockLogger).RegisterValidation(mockValidationRegistry) - mockLogger.EXPECT().Fatal(gomock.Any()).Times(1) + mockLogger.EXPECT().Fatalf(gomock.Any(), gomock.Any()).Times(1) // second registration of `apiKey` causes logger.Fatal invocation NewAPI(mockService, mockLogger).RegisterValidation(mockValidationRegistry) diff --git a/pkg/clients/validation_test.go b/pkg/clients/validation_test.go index f563d6a42..f91deb083 100644 --- a/pkg/clients/validation_test.go +++ b/pkg/clients/validation_test.go @@ -202,11 +202,27 @@ func TestValidate(t *testing.T) { t.Run(tt.name, func(t *testing.T) { _, result, err := p.Validate(context.TODO(), tt.method, tt.endpoint, tt.body()) - if err != nil && err.Error() != tt.expectedError.Error() { - t.Fatalf("Returned error doesn't match expected, obtained '%v' instead of '%v'", err, tt.expectedError) - } else if result != nil && errors.Is(result, tt.expectedResult) { - t.Fatalf("Returned validation errors don't match expected, obtained '%v' instead of '%v'", result, tt.expectedResult) - } else { + if err != nil { + if tt.expectedError == nil { + t.Fatalf("Unexpected error for '%s'", tt.name) + } + + if err.Error() != tt.expectedError.Error() { + t.Fatalf("Returned error doesn't match expected, obtained '%v' instead of '%v'", err, tt.expectedError) + } + + return + } + + if result != nil { + if tt.expectedResult == nil { + t.Fatalf("Unexpected result for '%s'", tt.name) + } + + if errors.Is(result, tt.expectedResult) { + t.Fatalf("Returned validation errors don't match expected, obtained '%v' instead of '%v'", result, tt.expectedResult) + } + return } }) From 6d31df5016ab38020b2a77654c194c3f04cde3fb Mon Sep 17 00:00:00 2001 From: barco Date: Thu, 18 Apr 2024 16:09:23 +0200 Subject: [PATCH 16/17] refactor: rename vldt to validationRegistry + enable validation --- pkg/web/router.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/web/router.go b/pkg/web/router.go index 4af466b9a..45ae09ce5 100644 --- a/pkg/web/router.go +++ b/pkg/web/router.go @@ -34,7 +34,7 @@ func NewRouter(idpConfig *idp.Config, schemasConfig *schemas.Config, rulesConfig monitor := ollyConfig.Monitor() tracer := ollyConfig.Tracer() - vldtr := validation.NewRegistry(tracer, monitor, logger) + validationRegistry := validation.NewRegistry(tracer, monitor, logger) middlewares := make(chi.Middlewares, 0) middlewares = append( @@ -58,7 +58,7 @@ func NewRouter(idpConfig *idp.Config, schemasConfig *schemas.Config, rulesConfig router = router.With( authorization.NewMiddleware( authorization.NewAuthorizer(externalConfig.Authorizer(), tracer, monitor, logger), monitor, logger).Authorize(), - vldtr.ValidationMiddleware, + validationRegistry.ValidationMiddleware, ).(*chi.Mux) status.NewAPI(tracer, monitor, logger).RegisterEndpoints(router) @@ -69,35 +69,35 @@ func NewRouter(idpConfig *idp.Config, schemasConfig *schemas.Config, rulesConfig logger, ) identitiesAPI.RegisterEndpoints(router) - // identitiesAPI.RegisterValidation(vldtr) + identitiesAPI.RegisterValidation(validationRegistry) clientsAPI := clients.NewAPI( clients.NewService(externalConfig.HydraAdmin(), tracer, monitor, logger), logger, ) clientsAPI.RegisterEndpoints(router) - // clientsAPI.RegisterValidation(vldtr) + clientsAPI.RegisterValidation(validationRegistry) idpAPI := idp.NewAPI( idp.NewService(idpConfig, tracer, monitor, logger), logger, ) idpAPI.RegisterEndpoints(router) - // idpAPI.RegisterValidation(vldtr) + idpAPI.RegisterValidation(validationRegistry) schemasAPI := schemas.NewAPI( schemas.NewService(schemasConfig, tracer, monitor, logger), logger, ) schemasAPI.RegisterEndpoints(router) - // schemasAPI.RegisterValidation(vldtr) + schemasAPI.RegisterValidation(validationRegistry) rulesAPI := rules.NewAPI( rules.NewService(rulesConfig, tracer, monitor, logger), logger, ) rulesAPI.RegisterEndpoints(router) - // rulesAPI.RegisterValidation(vldtr) + rulesAPI.RegisterValidation(validationRegistry) rolesAPI := roles.NewAPI( roles.NewService(externalConfig.OpenFGA(), wpool, tracer, monitor, logger), @@ -106,7 +106,7 @@ func NewRouter(idpConfig *idp.Config, schemasConfig *schemas.Config, rulesConfig logger, ) rolesAPI.RegisterEndpoints(router) - // rolesAPI.RegisterValidation(vldtr) + rolesAPI.RegisterValidation(validationRegistry) groupsAPI := groups.NewAPI( groups.NewService(externalConfig.OpenFGA(), wpool, tracer, monitor, logger), @@ -115,7 +115,7 @@ func NewRouter(idpConfig *idp.Config, schemasConfig *schemas.Config, rulesConfig logger, ) groupsAPI.RegisterEndpoints(router) - // groupsAPI.RegisterValidation(vldtr) + groupsAPI.RegisterValidation(validationRegistry) return tracing.NewMiddleware(monitor, logger).OpenTelemetry(router) } From 7e586513a24da79bf0859984fcc72d2253ccd1da Mon Sep 17 00:00:00 2001 From: barco Date: Thu, 18 Apr 2024 17:38:34 +0200 Subject: [PATCH 17/17] enhance: add comments to explain validation tags --- pkg/clients/validation.go | 13 ++++++++----- pkg/groups/handlers.go | 2 ++ pkg/identities/validation.go | 2 +- pkg/idp/third_party.go | 10 ++++++++++ pkg/idp/validation.go | 1 + pkg/roles/handlers.go | 1 + pkg/rules/validation.go | 7 +++++-- 7 files changed, 28 insertions(+), 8 deletions(-) diff --git a/pkg/clients/validation.go b/pkg/clients/validation.go index db9c392b6..01982e6af 100644 --- a/pkg/clients/validation.go +++ b/pkg/clients/validation.go @@ -17,11 +17,14 @@ import ( var ( oauth2ClientRules = map[string]string{ - "AllowedCorsOrigins": "omitempty,dive,required", - "Audience": "omitempty,dive,required", - "ClientName": "required", - "SubjectType": "omitempty,oneof=pairwise public", - "GrantTypes": "omitempty,dive,required", + // if not empy, validate every item is not nil and not empty + "AllowedCorsOrigins": "omitempty,dive,required", + "Audience": "omitempty,dive,required", + "GrantTypes": "omitempty,dive,required", + "ClientName": "required", + // if not empty, validate value is one of 'pairwise' and 'public' + "SubjectType": "omitempty,oneof=pairwise public", + // if not empty, validate value is one of 'client_secret_basic', 'client_secret_post', 'private_key_jwt' and 'none' "TokenEndpointAuthMethod": "omitempty,oneof=client_secret_basic client_secret_post private_key_jwt none", } ) diff --git a/pkg/groups/handlers.go b/pkg/groups/handlers.go index 09d9d7497..c83178134 100644 --- a/pkg/groups/handlers.go +++ b/pkg/groups/handlers.go @@ -26,6 +26,7 @@ const ( ) type UpdateRolesRequest struct { + // validate slice is not nil, and each item is not nil Roles []string `json:"roles" validate:"required,dive,required"` } @@ -35,6 +36,7 @@ type Permission struct { } type UpdatePermissionsRequest struct { + // validate slice is not nil, and each item is not nil Permissions []Permission `json:"permissions" validate:"required,dive,required"` } diff --git a/pkg/identities/validation.go b/pkg/identities/validation.go index b52465a25..c65289f83 100644 --- a/pkg/identities/validation.go +++ b/pkg/identities/validation.go @@ -19,8 +19,8 @@ var ( "Credentials": "required", } - // mutually exclusive fields credentialsRules = map[string]string{ + // mutually exclusive fields "Oidc": "required_without=Password,excluded_with=Password", "Password": "required_without=Oidc,excluded_with=Oidc", } diff --git a/pkg/idp/third_party.go b/pkg/idp/third_party.go index 569d1f914..087a10882 100644 --- a/pkg/idp/third_party.go +++ b/pkg/idp/third_party.go @@ -31,6 +31,7 @@ type Configuration struct { // - dingtalk // - linkedin // - patreon + // validate that Provider is not nil and not empty string, and its value is one of the supported ones Provider string `json:"provider" yaml:"provider" validate:"required,supported_provider"` // Label represents an optional label which can be used in the UI generation. @@ -44,21 +45,25 @@ type Configuration struct { // IssuerURL is the OpenID Connect Server URL. You can leave this empty if `provider` is not set to `generic`. // If set, neither `auth_url` nor `token_url` are required. + // validate that this field is required only when Provider field == "generic" IssuerURL string `json:"issuer_url" yaml:"issuer_url" validate:"required_if=Provider generic"` // AuthURL is the authorize url, typically something like: https://example.org/oauth2/auth // Should only be used when the OAuth2 / OpenID Connect server is not supporting OpenID Connect Discovery and when // `provider` is set to `generic`. + // validate that this field is required only when Provider field == "generic" AuthURL string `json:"auth_url" yaml:"auth_url" validate:"required_if=Provider generic"` // TokenURL is the token url, typically something like: https://example.org/oauth2/token // Should only be used when the OAuth2 / OpenID Connect server is not supporting OpenID Connect Discovery and when // `provider` is set to `generic`. + // validate that this field is required only when Provider field == "generic" TokenURL string `json:"token_url" yaml:"token_url" validate:"required_if=Provider generic"` // Tenant is the Azure AD Tenant to use for authentication, and must be set when `provider` is set to `microsoft`. // Can be either `common`, `organizations`, `consumers` for a multitenant application or a specific tenant like // `8eaef023-2b34-4da1-9baa-8bc8c9d6a490` or `contoso.onmicrosoft.com`. + // validate that this field is required only when Provider field == "microsoft" Tenant string `json:"microsoft_tenant" yaml:"microsoft_tenant" validate:"required_if=Provider microsoft"` // SubjectSource is a flag which controls from which endpoint the subject identifier is taken by microsoft provider. @@ -66,23 +71,28 @@ type Configuration struct { // If the value is `userinfo` then the subject identifier is taken from sub field of userinfo standard endpoint response. // If the value is `me` then the `id` field of https://graph.microsoft.com/v1.0/me response is taken as subject. // The default is `userinfo`. + // validate that SubjectSource is either "userinfo" or "me" SubjectSource string `json:"subject_source" yaml:"subject_source" validate:"oneof=userinfo me"` // TeamId is the Apple Developer Team ID that's needed for the `apple` `provider` to work. // It can be found Apple Developer website and combined with `apple_private_key` and `apple_private_key_id` // is used to generate `client_secret` + // validate that this field is required only when Provider field == "apple" TeamId string `json:"apple_team_id" yaml:"apple_team_id" validate:"required_if=Provider apple"` // PrivateKeyId is the private Apple key identifier. Keys can be generated via developer.apple.com. // This key should be generated with the `Sign In with Apple` option checked. // This is needed when `provider` is set to `apple` + // validate that this field is required only when Provider field == "apple" PrivateKeyId string `json:"apple_private_key_id" yaml:"apple_private_key_id" validate:"required_if=Provider apple"` // PrivateKeyId is the Apple private key identifier that can be downloaded during key generation. // This is needed when `provider` is set to `apple` + // validate that this field is required only when Provider field == "apple" PrivateKey string `json:"apple_private_key" yaml:"apple_private_key" validate:"required_if=Provider apple"` // Scope specifies optional requested permissions. + // validate that Scope is either nil or a slice with non-empty strings Scope []string `json:"scope" yaml:"scope" validate:"omitempty,dive,required"` // Mapper specifies the JSONNet code snippet which uses the OpenID Connect Provider's data (e.g. GitHub or Google diff --git a/pkg/idp/validation.go b/pkg/idp/validation.go index 2ae4f0b97..43f8ef484 100644 --- a/pkg/idp/validation.go +++ b/pkg/idp/validation.go @@ -20,6 +20,7 @@ type PayloadValidator struct { } func (p *PayloadValidator) setupValidator() { + // validate Provider to be one of the supported ones p.validator.RegisterAlias("supported_provider", "oneof=generic google github githubapp gitlab microsoft discord slack facebook auth0 vk yandex apple spotify netid dingtalk linkedin patreon") } diff --git a/pkg/roles/handlers.go b/pkg/roles/handlers.go index efdfd4315..fd1ccfc51 100644 --- a/pkg/roles/handlers.go +++ b/pkg/roles/handlers.go @@ -30,6 +30,7 @@ type Permission struct { } type UpdatePermissionsRequest struct { + // validate slice is not nil, and each item is not nil Permissions []Permission `json:"permissions" validate:"required,dive,required"` } diff --git a/pkg/rules/validation.go b/pkg/rules/validation.go index 537bee5b9..d58e7592d 100644 --- a/pkg/rules/validation.go +++ b/pkg/rules/validation.go @@ -17,14 +17,17 @@ import ( var ( ruleRules = map[string]string{ + // validate slice is not empty "Authenticators": "required,min=1,dive,required", "Authorizer": "required", "Match": "required", - "Mutators": "omitempty,dive,required", - "Upstream": "omitempty,required", + // if not empy, validate every item is not nil and not empty + "Mutators": "omitempty,dive,required", + "Upstream": "omitempty,required", } ruleMatchRules = map[string]string{ + // validate slice is not empty, and each item is a valid uppercase http method "Methods": "required,min=1,dive,httpmethod", "Url": "required", }