Skip to content

Commit

Permalink
Validate config before saving
Browse files Browse the repository at this point in the history
  • Loading branch information
jhrozek committed Jun 3, 2024
1 parent b5f59ba commit 1ba11d6
Show file tree
Hide file tree
Showing 5 changed files with 88 additions and 3 deletions.
11 changes: 11 additions & 0 deletions internal/providers/dockerhub/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
22 changes: 22 additions & 0 deletions internal/providers/github/manager/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
11 changes: 11 additions & 0 deletions internal/providers/manager/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}

Expand Down
33 changes: 30 additions & 3 deletions internal/providers/manager/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
},
}

Expand All @@ -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 != "" {
Expand Down
14 changes: 14 additions & 0 deletions internal/providers/manager/mock/manager.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 1ba11d6

Please sign in to comment.