Validate provider config when creating the OAuth provider or the GitHub App provider.

internal/controlplane/handlers_oauth.go

@@ -274,10 +274,15 @@ func (s *Server) processOAuthCallback(ctx context.Context, w http.ResponseWriter
 		return fmt.Errorf("error encoding token: %w", err)
 	}
 
+	var errConfig providers.ErrProviderInvalidConfig
+
 	p, err := s.sessionService.CreateProviderFromSessionState(ctx, db.ProviderClass(provider), &encryptedToken, state)
 	if db.ErrIsUniqueViolation(err) {
 		// todo: update config?
 		zerolog.Ctx(ctx).Info().Str("provider", provider).Msg("Provider already exists")
+	} else if errors.As(err, &errConfig) {
+		return newHttpError(http.StatusBadRequest, "Invalid provider config").SetContents(
+			"The provider configuration is invalid: " + errConfig.Details)
 	} else if err != nil {
 		return fmt.Errorf("error creating provider: %w", err)
 	}
@@ -349,8 +354,13 @@ func (s *Server) processAppCallback(ctx context.Context, w http.ResponseWriter,
 
 		logger.BusinessRecord(ctx).Project = stateData.ProjectID
 
+		var confErr providers.ErrProviderInvalidConfig
 		_, err = s.ghProviders.CreateGitHubAppProvider(ctx, *token, stateData, installationID, state)
 		if err != nil {
+			if errors.As(err, &confErr) {
+				return newHttpError(http.StatusBadRequest, "Invalid provider config").SetContents(
+					"The provider configuration is invalid: " + confErr.Details)
+			}
 			if errors.Is(err, service.ErrInvalidTokenIdentity) {
 				return newHttpError(http.StatusForbidden, "User token mismatch").SetContents(
 					"The provided login token was associated with a different GitHub user.")

internal/controlplane/handlers_oauth_test.go

@@ -456,6 +456,12 @@ func TestProviderCallback(t *testing.T) {
 		store.EXPECT().CreateProvider(gomock.Any(), gomock.Any()).Return(db.Provider{}, nil)
 	}
 
+	withProviderNotFound := func(store *mockdb.MockStore) {
+		store.EXPECT().GetParentProjects(gomock.Any(), projectID).Return([]uuid.UUID{projectID}, nil)
+		store.EXPECT().FindProviders(gomock.Any(), gomock.Any()).
+			Return([]db.Provider{}, nil)
+	}
+
 	testCases := []struct {
 		name                       string
 		redirectUrl                string
@@ -464,6 +470,7 @@ func TestProviderCallback(t *testing.T) {
 		storeMockSetup             func(store *mockdb.MockStore)
 		projectIDBySessionNumCalls int
 		err                        string
+		config                     []byte
 	}{{
 		name:                       "Success",
 		redirectUrl:                "http://localhost:8080",
@@ -498,6 +505,16 @@ func TestProviderCallback(t *testing.T) {
 		storeMockSetup: func(store *mockdb.MockStore) {
 			withProviderCreate(store)
 		},
+	}, {
+		name:                       "Config does not validate",
+		redirectUrl:                "http://localhost:8080",
+		remoteUser:                 sql.NullString{Valid: true, String: "31337"},
+		projectIDBySessionNumCalls: 2,
+		storeMockSetup: func(store *mockdb.MockStore) {
+			withProviderNotFound(store)
+		},
+		config: []byte(`{"missing_github_key": true}`),
+		code:   http.StatusBadRequest,
 	}}
 
 	for _, tt := range testCases {
@@ -636,7 +653,8 @@ func TestProviderCallback(t *testing.T) {
 						RawMessage: serialized,
 						Valid:      true,
 					},
-					RemoteUser: tc.remoteUser,
+					RemoteUser:     tc.remoteUser,
+					ProviderConfig: tc.config,
 				}, nil).Times(tc.projectIDBySessionNumCalls)
 
 			if tc.code < http.StatusBadRequest {
@@ -765,6 +783,27 @@ func TestHandleGitHubAppCallback(t *testing.T) {
 				t.Helper()
 				assert.Equal(t, 403, resp.Code)
 			},
+		}, {
+			name:  "Invalid config",
+			state: validState,
+			buildStubs: func(store *mockdb.MockStore, service *mockprovsvc.MockGitHubProviderService, _ *mockgh.MockClientService) {
+				service.EXPECT().
+					ValidateGitHubInstallationId(gomock.Any(), gomock.Any(), installationID).
+					Return(nil)
+				store.EXPECT().
+					GetProjectIDBySessionState(gomock.Any(), validState).
+					Return(db.GetProjectIDBySessionStateRow{
+						ProjectID: uuid.New(),
+					}, nil)
+				service.EXPECT().
+					CreateGitHubAppProvider(gomock.Any(), gomock.Any(), gomock.Any(), installationID, gomock.Any()).
+					Return(nil, providers.NewErrProviderInvalidConfig("invalid config"))
+			},
+			checkResponse: func(t *testing.T, resp httptest.ResponseRecorder) {
+				t.Helper()
+				assert.Equal(t, http.StatusBadRequest, resp.Code)
+				assert.Contains(t, resp.Body.String(), "The provider configuration is invalid: invalid config")
+			},
 		}, {
 			name:        "Request to install",
 			state:       validState,

internal/controlplane/handlers_providers.go

@@ -62,11 +62,15 @@ func (s *Server) CreateProvider(
 		zerolog.Ctx(ctx).Debug().Msg("no provider provConfig, will use default")
 	}
 
+	var configErr providers.ErrProviderInvalidConfig
 	dbProv, err := s.providerManager.CreateFromConfig(
 		ctx, db.ProviderClass(provider.GetClass()), projectID, provider.Name, provConfig)
 	if db.ErrIsUniqueViolation(err) {
 		zerolog.Ctx(ctx).Error().Err(err).Msg("provider already exists")
 		return nil, util.UserVisibleError(codes.AlreadyExists, "provider already exists")
+	} else if errors.As(err, &configErr) {
+		zerolog.Ctx(ctx).Error().Err(err).Msg("provider config does not validate")
+		return nil, util.UserVisibleError(codes.InvalidArgument, "invalid provider config: "+configErr.Details)
 	} else if err != nil {
 		return nil, status.Errorf(codes.Internal, "error creating provider: %v", err)
 	}

internal/controlplane/handlers_providers_test.go

@@ -447,6 +447,12 @@ func TestCreateProviderFailures(t *testing.T) {
 		})
 		assert.Error(t, err)
 		require.ErrorContains(t, err, "error validating provider config: auto_registration: invalid entity type: blah")
+
+		// test special-casing of the invalid config error
+		st, ok := status.FromError(err)
+		require.True(t, ok)
+		assert.Equal(t, codes.InvalidArgument, st.Code())
+		require.ErrorContains(t, err, "invalid provider config")
 	})
 }
 

internal/providers/github/service/service.go

@@ -262,6 +262,11 @@ func (p *ghProviderService) CreateGitHubAppProvider(
 			return nil, fmt.Errorf("error getting provider config: %w", err)
 		}
 
+		_, _, err = clients.ParseV1AppConfig(finalConfig)
+		if err != nil {
+			return nil, providers.NewErrProviderInvalidConfig(err.Error())
+		}
+
 		provider, err := createGitHubApp(
 			ctx,
 			qtx,

internal/providers/github/service/service_test.go

@@ -46,6 +46,7 @@ import (
 	mockcrypto "github.com/stacklok/minder/internal/crypto/mock"
 	"github.com/stacklok/minder/internal/db"
 	"github.com/stacklok/minder/internal/db/embedded"
+	"github.com/stacklok/minder/internal/providers"
 	"github.com/stacklok/minder/internal/providers/credentials"
 	"github.com/stacklok/minder/internal/providers/github/clients"
 	mockclients "github.com/stacklok/minder/internal/providers/github/clients/mock"
@@ -276,6 +277,63 @@ func TestProviderService_VerifyProviderTokenIdentity(t *testing.T) {
 	require.ErrorIs(t, err, ErrInvalidTokenIdentity)
 }
 
+func TestProviderService_CreateGitHubOAuthProviderWithInvalidConfig(t *testing.T) {
+	t.Parallel()
+
+	const (
+		installationID = 123
+		accountLogin   = "test-user"
+		accountID      = 456
+		stateNonce     = "test-githubapp-nonce"
+	)
+
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	pvtKeyFile := testCreatePrivateKeyFile(t)
+	defer os.Remove(pvtKeyFile.Name())
+	cfg := &server.ProviderConfig{
+		GitHubApp: &server.GitHubAppConfig{
+			PrivateKey: pvtKeyFile.Name(),
+		},
+	}
+
+	clientFactory := mockclients.NewMockGitHubClientFactory(ctrl)
+
+	provSvc, mocks := testNewGitHubProviderService(t, ctrl, cfg, nil, clientFactory)
+	dbproj, err := mocks.fakeStore.CreateProject(context.Background(),
+		db.CreateProjectParams{
+			Name:     "test",
+			Metadata: []byte(`{}`),
+		})
+	require.NoError(t, err)
+
+	mocks.svcMock.EXPECT().
+		GetInstallation(gomock.Any(), int64(installationID), gomock.Any()).
+		Return(&github.Installation{
+			Account: &github.User{
+				Login: github.String(accountLogin),
+				ID:    github.Int64(accountID),
+			},
+		}, nil, nil)
+
+	dbProv, err := provSvc.CreateGitHubAppProvider(
+		context.Background(), oauth2.Token{},
+		db.GetProjectIDBySessionStateRow{
+			ProjectID: dbproj.ID,
+			RemoteUser: sql.NullString{
+				Valid:  true,
+				String: strconv.Itoa(accountID),
+			},
+			ProviderConfig: []byte(`{ "auto_registration": { "entities": { "blah": {"enabled": true }}}, "github-app": {}}`),
+		},
+		installationID,
+		stateNonce)
+	require.Error(t, err)
+	require.ErrorAs(t, err, &providers.ErrProviderInvalidConfig{})
+	require.Nil(t, dbProv)
+}
+
 func TestProviderService_CreateGitHubAppProvider(t *testing.T) {
 	t.Parallel()
 

internal/providers/manager/manager.go

@@ -156,7 +156,7 @@ func (p *providerManager) CreateFromConfig(
 
 	err = manager.ValidateConfig(ctx, providerClass, provConfig)
 	if err != nil {
-		return nil, fmt.Errorf("error validating provider config: %w", err)
+		return nil, providers.NewErrProviderInvalidConfig(err.Error())
 	}
 
 	return p.store.Create(ctx, providerClass, name, projectID, provConfig)

internal/providers/manager/manager_test.go

@@ -61,7 +61,7 @@ func TestProviderManager_CreateFromConfig(t *testing.T) {
 			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",
+			ExpectedError:     "invalid provider configuration",
 			ValidateConfigErr: true,
 		},
 	}

internal/providers/providers.go

@@ -35,6 +35,22 @@ import (
 	provinfv1 "github.com/stacklok/minder/pkg/providers/v1"
 )
 
+// ErrProviderInvalidConfig is an error type which is returned when a provider configuration is invalid
+type ErrProviderInvalidConfig struct {
+	Details string
+}
+
+func (e ErrProviderInvalidConfig) Error() string {
+	return fmt.Sprintf("invalid provider configuration: %s", e.Details)
+}
+
+// NewErrProviderInvalidConfig returns a new instance of ErrProviderInvalidConfig with details
+// about the invalid configuration. This is meant for user-facing errors so that the only thing
+// displayed to the user is the details of the error.
+func NewErrProviderInvalidConfig(details string) ErrProviderInvalidConfig {
+	return ErrProviderInvalidConfig{Details: details}
+}
+
 // DBToPBType converts a database provider type to a protobuf provider type.
 func DBToPBType(t db.ProviderType) (minderv1.ProviderType, bool) {
 	switch t {