Validate config before provider creation (#3513)

Not validating provider config before provider creation can have bad
consequences such as not being able to delete such provider because a
provider must be instantiated before it's deleted.

Related: #3510

internal/controlplane/handlers_providers_test.go

@@ -183,30 +183,21 @@ func TestCreateProvider(t *testing.T) {
 				Class: string(db.ProviderClassGithubApp),
 			},
 		},
-		{
-			name:          "test-dockerhub-defaults",
-			providerClass: db.ProviderClassDockerhub,
-			expected: minder.Provider{
-				Name: "test-dockerhub-defaults",
-				Config: newPbStruct(t, map[string]interface{}{
-					"dockerhub": map[string]interface{}{},
-				}),
-				Class: string(db.ProviderClassDockerhub),
-			},
-		},
 		{
 			name:          "test-dockerhub-config",
 			providerClass: db.ProviderClassDockerhub,
 			userConfig: newPbStruct(t, map[string]interface{}{
 				"dockerhub": map[string]interface{}{
-					"key": "value",
+					"key":       "value",
+					"namespace": "myproject",
 				},
 			}),
 			expected: minder.Provider{
 				Name: "test-dockerhub-config",
 				Config: newPbStruct(t, map[string]interface{}{
 					"dockerhub": map[string]interface{}{
-						"key": "value",
+						"key":       "value",
+						"namespace": "myproject",
 					},
 				}),
 				Class: string(db.ProviderClassDockerhub),
@@ -370,6 +361,73 @@ func TestCreateProviderFailures(t *testing.T) {
 		require.True(t, ok)
 		assert.Equal(t, codes.AlreadyExists, st.Code())
 	})
+
+	t.Run("dockerhub-does-not-validate", func(t *testing.T) {
+		t.Parallel()
+
+		projectID := uuid.New()
+		projectIDStr := projectID.String()
+
+		ctrl := gomock.NewController(t)
+		t.Cleanup(ctrl.Finish)
+
+		fakeServer := testServer(t, ctrl)
+		providerName := "bad-dockerhub"
+
+		_, err := fakeServer.server.CreateProvider(context.Background(), &minder.CreateProviderRequest{
+			Context: &minder.Context{
+				Project:  &projectIDStr,
+				Provider: &providerName,
+			},
+			Provider: &minder.Provider{
+				Name:  providerName,
+				Class: string(db.ProviderClassDockerhub),
+				Config: newPbStruct(t, map[string]interface{}{
+					"dockerhub": map[string]interface{}{
+						"key": "value",
+					},
+				}),
+			},
+		})
+		assert.Error(t, err)
+		require.ErrorContains(t, err, "error validating DockerHub v1 provider config: namespace is required")
+	})
+
+	t.Run("github-app-does-not-validate", func(t *testing.T) {
+		t.Parallel()
+
+		projectID := uuid.New()
+		projectIDStr := projectID.String()
+
+		ctrl := gomock.NewController(t)
+		t.Cleanup(ctrl.Finish)
+
+		fakeServer := testServer(t, ctrl)
+		providerName := "bad-github-app"
+
+		_, err := fakeServer.server.CreateProvider(context.Background(), &minder.CreateProviderRequest{
+			Context: &minder.Context{
+				Project:  &projectIDStr,
+				Provider: &providerName,
+			},
+			Provider: &minder.Provider{
+				Name:  providerName,
+				Class: string(db.ProviderClassGithubApp),
+				Config: newPbStruct(t, map[string]interface{}{
+					"auto_registration": map[string]interface{}{
+						"entities": map[string]interface{}{
+							"blah": map[string]interface{}{
+								"enabled": true,
+							},
+						},
+					},
+					"github-app": map[string]interface{}{},
+				}),
+			},
+		})
+		assert.Error(t, err)
+		require.ErrorContains(t, err, "error validating provider config: auto_registration: invalid entity type: blah")
+	})
 }
 
 type partialCreateParamsMatcher struct {

internal/providers/dockerhub/dockerhub.go

@@ -99,7 +99,7 @@ func ParseV1Config(rawCfg json.RawMessage) (*minderv1.DockerHubProviderConfig, e
 
 	// Validate the config according to the protobuf validation rules.
 	if err := w.DockerHub.Validate(); err != nil {
-		return nil, fmt.Errorf("error validating GitHubOAuth v1 provider config: %w", err)
+		return nil, fmt.Errorf("error validating DockerHub v1 provider config: %w", err)
 	}
 
 	return w.DockerHub, nil

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

@@ -68,6 +68,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)
@@ -126,6 +127,11 @@ func (p *providerManager) CreateFromConfig(
 		return nil, fmt.Errorf("error getting provider config: %w", err)
 	}
 
+	err = manager.ValidateConfig(ctx, providerClass, provConfig)
+	if err != nil {
+		return nil, fmt.Errorf("error validating provider config: %w", err)
+	}
+
 	return p.store.Create(ctx, providerClass, name, projectID, provConfig)
 }
 

internal/providers/manager/manager_test.go

@@ -18,6 +18,7 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"testing"
 
 	"github.com/google/uuid"
@@ -35,10 +36,11 @@ func TestProviderManager_CreateFromConfig(t *testing.T) {
 	t.Parallel()
 
 	scenarios := []struct {
-		Name          string
-		Provider      *db.Provider
-		Config        json.RawMessage
-		ExpectedError string
+		Name              string
+		Provider          *db.Provider
+		Config            json.RawMessage
+		ExpectedError     string
+		ValidateConfigErr bool
 	}{
 		{
 			Name:          "CreateFromConfig returns error when provider class has no associated manager",
@@ -55,6 +57,13 @@ func TestProviderManager_CreateFromConfig(t *testing.T) {
 			Provider: providerWithClass(db.ProviderClassGithub, providerWithConfig(json.RawMessage(`{ github: { key: value} }`))),
 			Config:   json.RawMessage(`{ github: { key: value} }`),
 		},
+		{
+			Name:              "CreateFromConfig returns an error when the config is invalid",
+			Provider:          providerWithClass(db.ProviderClassGithub, providerWithConfig(json.RawMessage(`{ github: { key: value} }`))),
+			Config:            json.RawMessage(`{ github: { key: value} }`),
+			ExpectedError:     "error validating provider config",
+			ValidateConfigErr: true,
+		},
 	}
 
 	for _, scenario := range scenarios {
@@ -69,6 +78,11 @@ func TestProviderManager_CreateFromConfig(t *testing.T) {
 			classManager := mockmanager.NewMockProviderClassManager(ctrl)
 			classManager.EXPECT().GetSupportedClasses().Return([]db.ProviderClass{db.ProviderClassGithub}).MaxTimes(1)
 			classManager.EXPECT().GetConfig(gomock.Any(), scenario.Provider.Class, gomock.Any()).Return(scenario.Config, nil).MaxTimes(1)
+			if scenario.ValidateConfigErr {
+				classManager.EXPECT().ValidateConfig(gomock.Any(), scenario.Provider.Class, scenario.Config).Return(fmt.Errorf("invalid config")).MaxTimes(1)
+			} else {
+				classManager.EXPECT().ValidateConfig(gomock.Any(), scenario.Provider.Class, scenario.Config).Return(nil).MaxTimes(1)
+			}
 
 			expectedProvider := providerWithClass(scenario.Provider.Class, providerWithConfig(scenario.Config))
 			store.EXPECT().Create(gomock.Any(), scenario.Provider.Class, scenario.Provider.Name, scenario.Provider.ProjectID, scenario.Config).Return(expectedProvider, nil).MaxTimes(1)