Add ProviderManager, make provider deletion generic

database/query/providers.sql

@@ -20,6 +20,9 @@ LIMIT 1;
 -- name: GetProviderByID :one
 SELECT * FROM providers WHERE id = $1;
 
+-- name: GetProviderByIDAndProject :one
+SELECT * FROM providers WHERE id = $1 AND project_id = $2;
+
 -- FindProviders allows us to take a trait and filter
 -- providers by it. It also optionally takes a name, in case we want to
 -- filter by name as well.

internal/controlplane/handlers_projects.go

@@ -194,7 +194,7 @@ func (s *Server) DeleteProject(
 		Str("operation", "delete").
 		Str("project", projectID.String()).
 		Logger()
-	if err := projects.DeleteProject(ctx, projectID, qtx, s.authzClient, s.ghProviders, l); err != nil {
+	if err := projects.DeleteProject(ctx, projectID, qtx, s.authzClient, s.providerManager, l); err != nil {
 		return nil, status.Errorf(codes.Internal, "error deleting project: %v", err)
 	}
 

internal/controlplane/handlers_providers.go

@@ -32,7 +32,6 @@ import (
 	"github.com/stacklok/minder/internal/util"
 	cursorutil "github.com/stacklok/minder/internal/util/cursor"
 	minderv1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
-	provinfv1 "github.com/stacklok/minder/pkg/providers/v1"
 )
 
 // GetProvider gets a given provider available in a specific project.
@@ -171,16 +170,11 @@ func (s *Server) DeleteProvider(
 		return nil, status.Errorf(codes.InvalidArgument, "provider name is required")
 	}
 
-	provider, err := s.providerStore.GetByNameInSpecificProject(ctx, projectID, providerName)
+	err := s.providerManager.DeleteByName(ctx, providerName, projectID)
 	if err != nil {
 		if errors.Is(err, sql.ErrNoRows) {
 			return nil, util.UserVisibleError(codes.NotFound, "provider not found")
 		}
-		return nil, status.Errorf(codes.Internal, "error getting provider: %v", err)
-	}
-
-	err = s.deleteProvider(ctx, provider, projectID)
-	if err != nil {
 		return nil, status.Errorf(codes.Internal, "error deleting provider: %v", err)
 	}
 
@@ -202,16 +196,11 @@ func (s *Server) DeleteProviderByID(
 		return nil, util.UserVisibleError(codes.InvalidArgument, "invalid provider ID")
 	}
 
-	provider, err := s.providerStore.GetByID(ctx, parsedProviderID)
+	err = s.providerManager.DeleteByID(ctx, parsedProviderID, projectID)
 	if err != nil {
 		if errors.Is(err, sql.ErrNoRows) {
 			return nil, util.UserVisibleError(codes.NotFound, "provider not found")
 		}
-		return nil, status.Errorf(codes.Internal, "error getting provider: %v", err)
-	}
-
-	err = s.deleteProvider(ctx, provider, projectID)
-	if err != nil {
 		return nil, status.Errorf(codes.Internal, "error deleting provider: %v", err)
 	}
 
@@ -220,44 +209,6 @@ func (s *Server) DeleteProviderByID(
 	}, nil
 }
 
-func (s *Server) deleteProvider(ctx context.Context, provider *db.Provider, projectID uuid.UUID) error {
-	pbOpts := []providers.ProviderBuilderOption{
-		providers.WithProviderMetrics(s.provMt),
-		providers.WithRestClientCache(s.restClientCache),
-	}
-
-	p, err := providers.GetProviderBuilder(ctx, *provider, s.store, s.cryptoEngine, &s.cfg.Provider,
-		s.fallbackTokenClient, pbOpts...)
-	if err != nil {
-		return status.Errorf(codes.Internal, "cannot get provider builder: %v", err)
-	}
-
-	// If the provider is a GitHub provider with a valid credential, delete all repositories associated with the provider
-	if p.Implements(db.ProviderTypeGithub) &&
-		providers.GetCredentialStateForProvider(ctx, *provider, s.store, s.cryptoEngine, &s.cfg.Provider) ==
-			provinfv1.CredentialStateSet {
-		client, err := p.GetGitHub()
-		if err != nil {
-			return status.Errorf(codes.Internal, "error creating github provider: %v", err)
-		}
-
-		// Delete all repositories associated with the provider and remove the webhooks
-		err = s.repos.DeleteRepositoriesByProvider(ctx, client, provider.Name, projectID)
-		if err != nil {
-			// Don't fail the deletion if the repositories cannot be deleted or webhook cannot be removed
-			// The repositories will still be deleted by a cascade delete in the database
-			zerolog.Ctx(ctx).Error().Err(err).Str("projectID", projectID.String()).Msg("error deleting repositories")
-		}
-	}
-
-	// Delete the provider itself
-	err = s.ghProviders.DeleteProvider(ctx, provider)
-	if err != nil {
-		return status.Errorf(codes.Internal, "error deleting provider: %v", err)
-	}
-	return nil
-}
-
 func protobufProviderImplementsFromDB(ctx context.Context, p db.Provider) []minderv1.ProviderType {
 	impls := make([]minderv1.ProviderType, 0, len(p.Implements))
 	for _, i := range p.Implements {

internal/controlplane/handlers_providers_test.go

@@ -22,6 +22,7 @@ import (
 	"github.com/google/uuid"
 	"github.com/lestrrat-go/jwx/v2/jwt/openid"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"go.uber.org/mock/gomock"
 	"golang.org/x/oauth2"
 
@@ -33,8 +34,10 @@ import (
 	"github.com/stacklok/minder/internal/db"
 	"github.com/stacklok/minder/internal/engine"
 	"github.com/stacklok/minder/internal/providers"
+	ghmanager "github.com/stacklok/minder/internal/providers/github/manager"
 	mockgh "github.com/stacklok/minder/internal/providers/github/mock"
 	mockprovsvc "github.com/stacklok/minder/internal/providers/github/service/mock"
+	"github.com/stacklok/minder/internal/providers/manager"
 	"github.com/stacklok/minder/internal/providers/ratecache"
 	mockghrepo "github.com/stacklok/minder/internal/repositories/github/mock"
 	minder "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
@@ -55,12 +58,13 @@ func TestDeleteProvider(t *testing.T) {
 	defer ctrl.Finish()
 
 	providerName := "test-provider"
+	providerID := uuid.New()
 	projectID := uuid.New()
 	projectIDStr := projectID.String()
 	accessToken := "test-token"
 
 	mockProvidersSvc := mockprovsvc.NewMockGitHubProviderService(ctrl)
-	mockProvidersSvc.EXPECT().DeleteProvider(gomock.Any(), gomock.Any()).Return(nil)
+	mockProvidersSvc.EXPECT().DeleteInstallation(gomock.Any(), gomock.Any()).Return(nil)
 
 	mockCryptoEngine := mockcrypto.NewMockEngine(ctrl)
 	mockCryptoEngine.EXPECT().
@@ -74,14 +78,20 @@ func TestDeleteProvider(t *testing.T) {
 		Implements: []db.ProviderType{
 			db.ProviderTypeGithub,
 		},
+		ID:         providerID,
 		Version:    provinfv1.V1,
 		Definition: json.RawMessage(`{"github-app": {}}`),
+		Class:      db.ProviderClassGithubApp,
 	}, nil)
 	mockStore.EXPECT().
 		GetAccessTokenByProjectID(gomock.Any(), gomock.Any()).
 		Return(db.ProviderAccessToken{
 			EncryptedToken: "encryptedToken",
 		}, nil).AnyTimes()
+	mockStore.EXPECT().DeleteProvider(gomock.Any(), db.DeleteProviderParams{
+		ID:        providerID,
+		ProjectID: projectID,
+	}).Return(nil)
 
 	mockRepoSvc := mockghrepo.NewMockRepositoryService(ctrl)
 	mockRepoSvc.EXPECT().DeleteRepositoriesByProvider(gomock.Any(), gomock.Any(), providerName, projectID).Return(nil)
@@ -94,13 +104,30 @@ func TestDeleteProvider(t *testing.T) {
 
 	clientCache.Set("", accessToken, db.ProviderTypeGithub, gh)
 
+	// I am not replacing provider store with a stub since I want to reuse
+	// these tests to test the logic in GitHubProviderClassManager
+	providerStore := providers.NewProviderStore(mockStore)
+	githubProviderManager := ghmanager.NewGitHubProviderClassManager(
+		clientCache,
+		nil,
+		&serverconfig.ProviderConfig{},
+		nil,
+		mockCryptoEngine,
+		mockRepoSvc,
+		mockStore,
+		mockProvidersSvc,
+	)
+	providerManager, err := manager.NewProviderManager(providerStore, githubProviderManager)
+	require.NoError(t, err)
+
 	server := Server{
 		cryptoEngine:    mockCryptoEngine,
 		store:           mockStore,
 		ghProviders:     mockProvidersSvc,
 		repos:           mockRepoSvc,
 		authzClient:     authzClient,
-		providerStore:   providers.NewProviderStore(mockStore),
+		providerStore:   providerStore,
+		providerManager: providerManager,
 		restClientCache: clientCache,
 		cfg:             &serverconfig.Config{},
 	}
@@ -142,23 +169,33 @@ func TestDeleteProviderByID(t *testing.T) {
 	accessToken := "test-token"
 
 	mockProvidersSvc := mockprovsvc.NewMockGitHubProviderService(ctrl)
-	mockProvidersSvc.EXPECT().DeleteProvider(gomock.Any(), gomock.Any()).Return(nil)
+	mockProvidersSvc.EXPECT().DeleteInstallation(gomock.Any(), gomock.Any()).Return(nil)
 
 	mockCryptoEngine := mockcrypto.NewMockEngine(ctrl)
 	mockCryptoEngine.EXPECT().
 		DecryptOAuthToken(gomock.Any()).
 		Return(oauth2.Token{AccessToken: accessToken}, nil).AnyTimes()
 
 	mockStore := mockdb.NewMockStore(ctrl)
-	mockStore.EXPECT().GetProviderByID(gomock.Any(), providerID).Return(db.Provider{
+	params := db.GetProviderByIDAndProjectParams{
+		ID:        providerID,
+		ProjectID: projectID,
+	}
+	mockStore.EXPECT().GetProviderByIDAndProject(gomock.Any(), params).Return(db.Provider{
 		Name:      providerName,
+		ID:        providerID,
 		ProjectID: projectID,
 		Implements: []db.ProviderType{
 			db.ProviderTypeGithub,
 		},
 		Version:    provinfv1.V1,
 		Definition: json.RawMessage(`{"github-app": {}}`),
+		Class:      db.ProviderClassGithubApp,
 	}, nil)
+	mockStore.EXPECT().DeleteProvider(gomock.Any(), db.DeleteProviderParams{
+		ID:        providerID,
+		ProjectID: projectID,
+	}).Return(nil)
 	mockStore.EXPECT().
 		GetAccessTokenByProjectID(gomock.Any(), gomock.Any()).
 		Return(db.ProviderAccessToken{
@@ -176,13 +213,28 @@ func TestDeleteProviderByID(t *testing.T) {
 
 	clientCache.Set("", accessToken, db.ProviderTypeGithub, gh)
 
+	providerStore := providers.NewProviderStore(mockStore)
+	githubProviderManager := ghmanager.NewGitHubProviderClassManager(
+		clientCache,
+		nil,
+		&serverconfig.ProviderConfig{},
+		nil,
+		mockCryptoEngine,
+		mockRepoSvc,
+		mockStore,
+		mockProvidersSvc,
+	)
+	providerManager, err := manager.NewProviderManager(providerStore, githubProviderManager)
+	require.NoError(t, err)
+
 	server := Server{
 		cryptoEngine:    mockCryptoEngine,
 		store:           mockStore,
 		ghProviders:     mockProvidersSvc,
 		repos:           mockRepoSvc,
 		authzClient:     authzClient,
-		providerStore:   providers.NewProviderStore(mockStore),
+		providerStore:   providerStore,
+		providerManager: providerManager,
 		restClientCache: clientCache,
 		cfg:             &serverconfig.Config{},
 	}

internal/controlplane/handlers_user.go

@@ -166,7 +166,7 @@ func (s *Server) DeleteUser(ctx context.Context,
 
 	subject := token.Subject()
 
-	err = DeleteUser(ctx, s.store, s.authzClient, s.ghProviders, subject)
+	err = DeleteUser(ctx, s.store, s.authzClient, s.providerManager, subject)
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "failed to delete user from database: %v", err)
 	}

internal/controlplane/identity_events.go

@@ -31,7 +31,7 @@ import (
 	serverconfig "github.com/stacklok/minder/internal/config/server"
 	"github.com/stacklok/minder/internal/db"
 	"github.com/stacklok/minder/internal/projects"
-	"github.com/stacklok/minder/internal/providers/github/service"
+	"github.com/stacklok/minder/internal/providers/manager"
 )
 
 const (
@@ -54,11 +54,11 @@ func SubscribeToIdentityEvents(
 	store db.Store,
 	authzClient authz.Client,
 	cfg *serverconfig.Config,
-	providerService service.GitHubProviderService,
+	providerManager manager.ProviderManager,
 ) error {
 	c := cron.New()
 	_, err := c.AddFunc(eventFetchInterval, func() {
-		HandleEvents(ctx, store, authzClient, cfg, providerService)
+		HandleEvents(ctx, store, authzClient, cfg, providerManager)
 	})
 	if err != nil {
 		return err
@@ -73,7 +73,7 @@ func HandleEvents(
 	store db.Store,
 	authzClient authz.Client,
 	cfg *serverconfig.Config,
-	providerService service.GitHubProviderService,
+	providerManager manager.ProviderManager,
 ) {
 	d := time.Now().Add(time.Duration(10) * time.Minute)
 	ctx, cancel := context.WithDeadline(ctx, d)
@@ -100,7 +100,7 @@ func HandleEvents(
 	}
 	for _, event := range events {
 		if event.Type == deleteAccountEventType {
-			err := DeleteUser(ctx, store, authzClient, providerService, event.UserId)
+			err := DeleteUser(ctx, store, authzClient, providerManager, event.UserId)
 			if err != nil {
 				zerolog.Ctx(ctx).Error().Msgf("events chron: error deleting user account: %v", err)
 			}
@@ -113,7 +113,7 @@ func DeleteUser(
 	ctx context.Context,
 	store db.Store,
 	authzClient authz.Client,
-	providerService service.GitHubProviderService,
+	providerManager manager.ProviderManager,
 	userId string,
 ) error {
 	l := zerolog.Ctx(ctx).With().
@@ -140,7 +140,7 @@ func DeleteUser(
 
 		for _, proj := range projs {
 			l.Debug().Str("project_id", proj.String()).Msg("cleaning up project")
-			if err := projects.CleanUpUnmanagedProjects(l.WithContext(ctx), userId, proj, qtx, authzClient, providerService); err != nil {
+			if err := projects.CleanUpUnmanagedProjects(l.WithContext(ctx), userId, proj, qtx, authzClient, providerManager); err != nil {
 				return db.User{}, fmt.Errorf("error deleting project %s: %v", proj.String(), err)
 			}
 		}

internal/controlplane/identity_events_test.go

@@ -35,7 +35,7 @@ import (
 	serverconfig "github.com/stacklok/minder/internal/config/server"
 	"github.com/stacklok/minder/internal/db"
 	"github.com/stacklok/minder/internal/db/embedded"
-	mockprovsvc "github.com/stacklok/minder/internal/providers/github/service/mock"
+	mockmanager "github.com/stacklok/minder/internal/providers/manager/mock"
 	minderv1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
 )
 
@@ -91,7 +91,8 @@ func TestHandleEvents(t *testing.T) {
 			},
 		},
 	}
-	HandleEvents(context.Background(), mockStore, &mock.NoopClient{Authorized: true}, &c, mockprovsvc.NewMockGitHubProviderService(ctrl))
+	mockProviderManager := mockmanager.NewMockProviderManager(ctrl)
+	HandleEvents(context.Background(), mockStore, &mock.NoopClient{Authorized: true}, &c, mockProviderManager)
 }
 
 func TestDeleteUserOneProject(t *testing.T) {
@@ -131,10 +132,10 @@ func TestDeleteUserOneProject(t *testing.T) {
 	}
 
 	ctrl := gomock.NewController(t)
-	providerService := mockprovsvc.NewMockGitHubProviderService(ctrl)
+	mockProviderManager := mockmanager.NewMockProviderManager(ctrl)
 
 	t.Log("Deleting user")
-	err = DeleteUser(ctx, store, &authzClient, providerService, u1.IdentitySubject)
+	err = DeleteUser(ctx, store, &authzClient, mockProviderManager, u1.IdentitySubject)
 	assert.NoError(t, err, "DeleteUser failed")
 
 	t.Log("Checking if user is removed from DB")
@@ -211,10 +212,10 @@ func TestDeleteUserMultiProjectMembership(t *testing.T) {
 	}
 
 	ctrl := gomock.NewController(t)
-	providerService := mockprovsvc.NewMockGitHubProviderService(ctrl)
+	mockProviderManager := mockmanager.NewMockProviderManager(ctrl)
 
 	t.Log("Deleting user")
-	err = DeleteUser(ctx, store, &authzClient, providerService, u1.IdentitySubject)
+	err = DeleteUser(ctx, store, &authzClient, mockProviderManager, u1.IdentitySubject)
 	assert.NoError(t, err, "DeleteUser failed")
 
 	t.Log("Checking if user1 is removed from DB")