Validate config before saving

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
+}

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
+}

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)
 }
 

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 != "" {