From 614f0d72dd544033f737004b9ab90ca5b97cbe4f Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Thu, 30 May 2024 22:49:39 +0200 Subject: [PATCH] Validate config before saving --- internal/providers/dockerhub/manager.go | 11 +++++++ internal/providers/github/manager/manager.go | 22 +++++++++++++ internal/providers/manager/manager.go | 11 +++++++ internal/providers/manager/manager_test.go | 33 ++++++++++++++++++-- internal/providers/manager/mock/manager.go | 14 +++++++++ 5 files changed, 88 insertions(+), 3 deletions(-) diff --git a/internal/providers/dockerhub/manager.go b/internal/providers/dockerhub/manager.go index 310f028787..f243c8599e 100644 --- a/internal/providers/dockerhub/manager.go +++ b/internal/providers/dockerhub/manager.go @@ -131,3 +131,14 @@ func (m *providerClassManager) GetConfig( // we just return the user config as is return userConfig, nil } + +func (m *providerClassManager) ValidateConfig( + _ context.Context, class db.ProviderClass, config json.RawMessage, +) error { + if !slices.Contains(m.GetSupportedClasses(), class) { + return fmt.Errorf("provider does not implement %s", string(class)) + } + + _, err := ParseV1Config(config) + return err +} diff --git a/internal/providers/github/manager/manager.go b/internal/providers/github/manager/manager.go index 8a0c7706b9..12f19a152e 100644 --- a/internal/providers/github/manager/manager.go +++ b/internal/providers/github/manager/manager.go @@ -320,3 +320,25 @@ func (g *githubProviderManager) GetConfig( return userConfig, nil } + +func (g *githubProviderManager) ValidateConfig( + _ context.Context, class db.ProviderClass, config json.RawMessage, +) error { + var err error + + if !slices.Contains(g.GetSupportedClasses(), class) { + return fmt.Errorf("provider does not implement %s", string(class)) + } + + // nolint:exhaustive // we really want handle only the two + switch class { + case db.ProviderClassGithub: + _, err = clients.ParseV1OAuthConfig(config) + case db.ProviderClassGithubApp: + _, _, err = clients.ParseV1AppConfig(config) + default: + return fmt.Errorf("unsupported provider class %s", class) + } + + return err +} diff --git a/internal/providers/manager/manager.go b/internal/providers/manager/manager.go index 3e21fabe23..8ffeb77c5c 100644 --- a/internal/providers/manager/manager.go +++ b/internal/providers/manager/manager.go @@ -72,6 +72,7 @@ type ProviderManager interface { // specific Provider class. The idea is that ProviderManager determines the // class of the Provider, and delegates to the appropraite ProviderClassManager type ProviderClassManager interface { + ValidateConfig(ctx context.Context, class db.ProviderClass, config json.RawMessage) error GetConfig(ctx context.Context, class db.ProviderClass, userConfig json.RawMessage) (json.RawMessage, error) // Build creates an instance of Provider based on the config in the DB Build(ctx context.Context, config *db.Provider) (v1.Provider, error) @@ -219,6 +220,16 @@ func (p *providerManager) PatchProviderConfig( return fmt.Errorf("error marshalling provider config: %w", err) } + manager, err := p.getClassManager(dbProvider.Class) + if err != nil { + return err + } + + err = manager.ValidateConfig(ctx, dbProvider.Class, mergedJSON) + if err != nil { + return fmt.Errorf("error validating provider config: %w", err) + } + return p.store.Update(ctx, dbProvider.ID, dbProvider.ProjectID, mergedJSON) } diff --git a/internal/providers/manager/manager_test.go b/internal/providers/manager/manager_test.go index 798927960d..72f9a2e84b 100644 --- a/internal/providers/manager/manager_test.go +++ b/internal/providers/manager/manager_test.go @@ -73,6 +73,7 @@ func TestProviderManager_PatchProviderConfig(t *testing.T) { Patch map[string]any MergedConfig json.RawMessage ExpectedError string + ValidConfig bool }{ { Name: "Enabling the auto_enroll field", @@ -84,6 +85,7 @@ func TestProviderManager_PatchProviderConfig(t *testing.T) { }, }, MergedConfig: json.RawMessage(`{ "auto_registration": { "enabled": ["repository"] }, "github-app": {}}`), + ValidConfig: true, }, { Name: "Disabling the auto_enroll field", @@ -95,6 +97,20 @@ func TestProviderManager_PatchProviderConfig(t *testing.T) { }, }, MergedConfig: json.RawMessage(`{ "auto_registration": { "enabled": [] }, "github-app": {}}`), + ValidConfig: true, + }, + { + Name: "Invalid config doesn't call the store.Update method", + Provider: githubAppProvider, + CurrentConfig: json.RawMessage(`{ "auto_registration": { "enabled": ["repository"] }, "github-app": {}}`), + Patch: map[string]any{ + "auto_registration": map[string]any{ + "enabled": []string{"my_little_pony"}, + }, + }, + MergedConfig: json.RawMessage(`{ "auto_registration": { "enabled": [] }, "github-app": {}}`), + ValidConfig: false, + ExpectedError: "error validating provider config: invalid config", }, } @@ -119,9 +135,20 @@ func TestProviderManager_PatchProviderConfig(t *testing.T) { Return(dbProvider, nil). Times(1) - store.EXPECT().Update(ctx, dbProvider.ID, dbProvider.ProjectID, &configMatcher{expected: scenario.MergedConfig}). - Return(nil). - Times(1) + if scenario.ValidConfig { + classManager.EXPECT().ValidateConfig(ctx, dbProvider.Class, gomock.Any()). + Return(nil). + Times(1) + store.EXPECT().Update(ctx, dbProvider.ID, dbProvider.ProjectID, &configMatcher{expected: scenario.MergedConfig}). + Return(nil). + Times(1) + } else { + classManager.EXPECT().ValidateConfig(ctx, dbProvider.Class, gomock.Any()). + Return(errors.New("invalid config")). + Times(1) + store.EXPECT().Update(ctx, dbProvider.ID, dbProvider.ProjectID, gomock.Any()). + Times(0) + } err = provManager.PatchProviderConfig(ctx, scenario.Provider.Name, scenario.Provider.ProjectID, scenario.Patch) if scenario.ExpectedError != "" { diff --git a/internal/providers/manager/mock/manager.go b/internal/providers/manager/mock/manager.go index 3652d0057a..ca15f79d3e 100644 --- a/internal/providers/manager/mock/manager.go +++ b/internal/providers/manager/mock/manager.go @@ -226,3 +226,17 @@ func (mr *MockProviderClassManagerMockRecorder) GetSupportedClasses() *gomock.Ca mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetSupportedClasses", reflect.TypeOf((*MockProviderClassManager)(nil).GetSupportedClasses)) } + +// ValidateConfig mocks base method. +func (m *MockProviderClassManager) ValidateConfig(ctx context.Context, class db.ProviderClass, config json.RawMessage) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ValidateConfig", ctx, class, config) + ret0, _ := ret[0].(error) + return ret0 +} + +// ValidateConfig indicates an expected call of ValidateConfig. +func (mr *MockProviderClassManagerMockRecorder) ValidateConfig(ctx, class, config any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ValidateConfig", reflect.TypeOf((*MockProviderClassManager)(nil).ValidateConfig), ctx, class, config) +}