Do not construct provider when validating user ID

Relates to #2845

When creating a provider, the code previously inserted the provider into
the database and then instantiated it in order to validate that the user
ID is correct. Alter the code so that it just instantiates the client
and some supporting structures to validate the user ID.

This client deliberately does not use the client cache in order to
simplify the code. Since this is the first usage of the client for that
provider, we will never find a suitable client in the cache anyway.

cmd/cli/app/quickstart/quickstart.go

@@ -36,7 +36,7 @@ import (
 	minderprov "github.com/stacklok/minder/cmd/cli/app/provider"
 	"github.com/stacklok/minder/cmd/cli/app/repo"
 	"github.com/stacklok/minder/internal/engine"
-	ghclient "github.com/stacklok/minder/internal/providers/github/oauth"
+	ghclient "github.com/stacklok/minder/internal/providers/github/clients"
 	"github.com/stacklok/minder/internal/util/cli"
 	minderv1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
 )

cmd/server/app/serve.go

@@ -125,7 +125,6 @@ var serveCmd = &cobra.Command{
 			controlplane.WithProviderMetrics(providerMetrics),
 			controlplane.WithAuthzClient(authzc),
 			controlplane.WithIdentityClient(idClient),
-			controlplane.WithRestClientCache(restClientCache),
 		}
 
 		tsmdw := logger.NewTelemetryStoreWMMiddleware(l)

internal/controlplane/common.go

@@ -24,13 +24,13 @@ import (
 
 	"google.golang.org/grpc/codes"
 
-	"github.com/stacklok/minder/internal/providers/github/oauth"
+	"github.com/stacklok/minder/internal/providers/github/clients"
 	"github.com/stacklok/minder/internal/util"
 	pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
 )
 
 const (
-	defaultProvider = oauth.Github
+	defaultProvider = clients.Github
 	githubURL       = "https://github.com"
 )
 

internal/controlplane/handlers_githubwebhooks_test.go

@@ -93,7 +93,7 @@ func (s *UnitTestSuite) TestHandleWebHookPing() {
 	defer ctrl.Finish()
 
 	mockStore := mockdb.NewMockStore(ctrl)
-	srv, evt := newDefaultServer(t, mockStore)
+	srv, evt := newDefaultServer(t, mockStore, nil)
 	srv.cfg.WebhookConfig.WebhookSecretFile = whSecretFile.Name()
 	defer evt.Close()
 
@@ -149,7 +149,7 @@ func (s *UnitTestSuite) TestHandleWebHookUnexistentRepository() {
 	defer ctrl.Finish()
 
 	mockStore := mockdb.NewMockStore(ctrl)
-	srv, evt := newDefaultServer(t, mockStore)
+	srv, evt := newDefaultServer(t, mockStore, nil)
 	defer evt.Close()
 
 	pq := testqueue.NewPassthroughQueue(t)
@@ -221,7 +221,7 @@ func (s *UnitTestSuite) TestHandleWebHookRepository() {
 	defer os.Remove(prevCredsFile.Name())
 
 	mockStore := mockdb.NewMockStore(ctrl)
-	srv, evt := newDefaultServer(t, mockStore)
+	srv, evt := newDefaultServer(t, mockStore, nil)
 	srv.cfg.WebhookConfig.WebhookSecret = "not-our-secret"
 	srv.cfg.WebhookConfig.PreviousWebhookSecretFile = prevCredsFile.Name()
 	defer evt.Close()
@@ -329,7 +329,7 @@ func (s *UnitTestSuite) TestHandleWebHookUnexistentRepoPackage() {
 	defer ctrl.Finish()
 
 	mockStore := mockdb.NewMockStore(ctrl)
-	srv, evt := newDefaultServer(t, mockStore)
+	srv, evt := newDefaultServer(t, mockStore, nil)
 	defer evt.Close()
 
 	pq := testqueue.NewPassthroughQueue(t)
@@ -397,7 +397,7 @@ func (s *UnitTestSuite) TestNoopWebhookHandler() {
 	defer ctrl.Finish()
 
 	mockStore := mockdb.NewMockStore(ctrl)
-	srv, evt := newDefaultServer(t, mockStore)
+	srv, evt := newDefaultServer(t, mockStore, nil)
 	defer evt.Close()
 
 	go func() {

internal/controlplane/handlers_oauth_test.go

@@ -47,10 +47,10 @@ import (
 	"github.com/stacklok/minder/internal/engine"
 	"github.com/stacklok/minder/internal/events"
 	"github.com/stacklok/minder/internal/providers"
+	mock_clients "github.com/stacklok/minder/internal/providers/github/clients/mock"
 	mockgh "github.com/stacklok/minder/internal/providers/github/mock"
 	ghService "github.com/stacklok/minder/internal/providers/github/service"
 	mockprovsvc "github.com/stacklok/minder/internal/providers/github/service/mock"
-	"github.com/stacklok/minder/internal/providers/ratecache"
 	pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
 	provinfv1 "github.com/stacklok/minder/pkg/providers/v1"
 )
@@ -313,7 +313,7 @@ func TestGetAuthorizationURL(t *testing.T) {
 			}
 			mockJwt := mockjwt.NewMockJwtValidator(ctrl)
 
-			server, err := NewServer(store, evt, c, mockJwt, providers.NewProviderStore(store))
+			server, err := NewServer(ctx, store, evt, c, mockJwt, providers.NewProviderStore(store))
 			require.NoError(t, err, "failed to create server")
 
 			res, err := server.GetAuthorizationURL(ctx, tc.req)
@@ -335,29 +335,31 @@ func TestProviderCallback(t *testing.T) {
 		code             int
 		existingProvider bool
 		err              string
-	}{{
-		name:             "Success",
-		redirectUrl:      "http://localhost:8080",
-		existingProvider: true,
-		code:             307,
-	}, {
-		name:             "Success with remote user",
-		redirectUrl:      "http://localhost:8080",
-		remoteUser:       sql.NullString{Valid: true, String: "31337"},
-		existingProvider: true,
-		code:             307,
-	}, {
-		name:             "Wrong remote userid",
-		remoteUser:       sql.NullString{Valid: true, String: "1234"},
-		existingProvider: true,
-		code:             403,
-		err:              "The provided login token was associated with a different GitHub user.\n",
-	}, {
-		name:             "No existing provider",
-		redirectUrl:      "http://localhost:8080",
-		existingProvider: false,
-		code:             307,
-	}}
+	}{
+		{
+			name:             "Success",
+			redirectUrl:      "http://localhost:8080",
+			existingProvider: true,
+			code:             307,
+		}, {
+			name:             "Success with remote user",
+			redirectUrl:      "http://localhost:8080",
+			remoteUser:       sql.NullString{Valid: true, String: "31337"},
+			existingProvider: true,
+			code:             307,
+		}, {
+			name:             "Wrong remote userid",
+			remoteUser:       sql.NullString{Valid: true, String: "1234"},
+			existingProvider: true,
+			code:             403,
+			err:              "The provided login token was associated with a different GitHub user.\n",
+		}, {
+			name:             "No existing provider",
+			redirectUrl:      "http://localhost:8080",
+			existingProvider: false,
+			code:             307,
+		},
+	}
 
 	for _, tt := range testCases {
 		tc := tt
@@ -395,22 +397,19 @@ func TestProviderCallback(t *testing.T) {
 			defer ctrl.Finish()
 			store := mockdb.NewMockStore(ctrl)
 
-			gh := mockgh.NewMockGitHub(ctrl)
-			gh.EXPECT().GetUserId(gomock.Any()).Return(int64(31337), nil).AnyTimes()
-
-			var opts []ServerOption
+			var clientFactory *mock_clients.MockGitHubClientFactory
 			if tc.remoteUser.String != "" {
-				// TODO: verfifyProviderTokenIdentity
-				cancelable, cancel := context.WithCancel(context.Background())
-				defer cancel()
-				clientCache := ratecache.NewRestClientCache(cancelable)
-				clientCache.Set("", "anAccessToken", db.ProviderTypeGithub, gh)
-				opts = []ServerOption{
-					WithRestClientCache(clientCache),
-				}
+				delegate := mockgh.NewMockDelegate(ctrl)
+				delegate.EXPECT().
+					GetUserId(gomock.Any()).
+					Return(int64(31337), nil)
+				clientFactory = mock_clients.NewMockGitHubClientFactory(ctrl)
+				clientFactory.EXPECT().
+					BuildOAuthClient(gomock.Any(), gomock.Any(), gomock.Any()).
+					Return(nil, delegate, nil)
 			}
 
-			s, _ := newDefaultServer(t, store, opts...)
+			s, _ := newDefaultServer(t, store, clientFactory)
 
 			var err error
 			encryptedUrlString, err := s.cryptoEngine.EncryptString(tc.redirectUrl)
@@ -426,8 +425,6 @@ func TestProviderCallback(t *testing.T) {
 			store.EXPECT().BeginTransaction().Return(&tx, nil)
 			store.EXPECT().GetQuerierWithTransaction(gomock.Any()).Return(store)
 
-			gh.EXPECT().GetUserId(gomock.Any()).Return(int64(31337), nil).AnyTimes()
-
 			store.EXPECT().GetProjectIDBySessionState(gomock.Any(), state).Return(
 				db.GetProjectIDBySessionStateRow{
 					ProjectID:   projectID,

internal/controlplane/handlers_repositories_test.go

@@ -32,8 +32,8 @@ import (
 	"github.com/stacklok/minder/internal/db"
 	"github.com/stacklok/minder/internal/engine"
 	"github.com/stacklok/minder/internal/providers"
+	ghprovider "github.com/stacklok/minder/internal/providers/github/clients"
 	mockgh "github.com/stacklok/minder/internal/providers/github/mock"
-	ghprovider "github.com/stacklok/minder/internal/providers/github/oauth"
 	"github.com/stacklok/minder/internal/providers/ratecache"
 	ghrepo "github.com/stacklok/minder/internal/repositories/github"
 	mockghrepo "github.com/stacklok/minder/internal/repositories/github/mock"

internal/controlplane/handlers_user_test.go

@@ -447,7 +447,7 @@ func TestDeleteUser_gRPC(t *testing.T) {
 				GoChannel: serverconfig.GoChannelEventConfig{},
 			})
 			require.NoError(t, err, "failed to setup eventer")
-			server, err := NewServer(mockStore, evt, &serverconfig.Config{
+			server, err := NewServer(ctx, mockStore, evt, &serverconfig.Config{
 				Auth: serverconfig.AuthConfig{
 					TokenKey: generateTokenKey(t),
 				},

internal/controlplane/server.go

@@ -137,10 +137,12 @@ func WithAuthzClient(c authz.Client) ServerOption {
 	}
 }
 
-// WithRestClientCache sets the rest client cache for the server
-func WithRestClientCache(c ratecache.RestClientCache) ServerOption {
+// WithGitHubProviderService sets the rest client cache for the server
+// This is used for unit testing, and it may be possible to remove this in future
+// with some refactoring.
+func WithGitHubProviderService(g service.GitHubProviderService) ServerOption {
 	return func(s *Server) {
-		s.restClientCache = c
+		s.ghProviders = g
 	}
 }
 
@@ -160,6 +162,7 @@ func WithServerMetrics(mt metrics.Metrics) ServerOption {
 
 // NewServer creates a new server instance
 func NewServer(
+	ctx context.Context,
 	store db.Store,
 	evt events.Publisher,
 	cfg *serverconfig.Config,
@@ -172,6 +175,9 @@ func NewServer(
 		return nil, fmt.Errorf("failed to create crypto engine: %w", err)
 	}
 
+	restClientCache := ratecache.NewRestClientCache(ctx)
+	defer restClientCache.Close()
+
 	whManager := webhooks.NewWebhookManager(cfg.WebhookConfig)
 	profileSvc := profiles.NewProfileService(evt)
 	mt := metrics.NewNoopMetrics()
@@ -184,6 +190,37 @@ func NewServer(
 	}
 	fallbackTokenClient := ghprov.NewFallbackTokenClient(cfg.Provider)
 
+	// See the comment on this struct. Ideally we would detach
+	// `s.makeProjectForGitHubApp` from the Server struct and pass it to
+	// NewGitHubProviderService directly. However, `s.makeProjectForGitHubApp`
+	// depends on the authz client, which gets set in `opts`, and we want to
+	// allow this to be overridden in the server options. Fixing this issue
+	// will require further refactoring.
+	projectFactoryWrapper := &service.ProjectFactoryWrapper{}
+	ghProviders := service.NewGithubProviderService(
+		store,
+		eng,
+		mt,
+		&cfg.Provider,
+		projectFactoryWrapper,
+		ghClientFactory,
+	)
+
+	githubProviderManager := ghmanager.NewGitHubProviderClassManager(
+		restClientCache,
+		ghClientFactory,
+		&cfg.Provider,
+		fallbackTokenClient,
+		eng,
+		whManager,
+		store,
+		ghProviders,
+	)
+	providerManager, err := manager.NewProviderManager(providerStore, githubProviderManager)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create provider manager: %w", err)
+	}
+
 	s := &Server{
 		store:               store,
 		cfg:                 cfg,
@@ -200,6 +237,10 @@ func NewServer(
 		featureFlags:        openfeature.NewClient(cfg.Flags.AppName),
 		ghClient:            &ghprov.ClientServiceImplementation{},
 		fallbackTokenClient: fallbackTokenClient,
+		ghProviders:         ghProviders,
+		providerManager:     providerManager,
+		repos:               github.NewRepositoryService(whManager, store, evt, providerManager),
+		restClientCache:     restClientCache,
 		// TODO: this currently always returns authorized as a transitionary measure.
 		// When OpenFGA is fully rolled out, we may want to make this a hard error or set to false.
 		authzClient: &mock.NoopClient{Authorized: true},
@@ -213,26 +254,10 @@ func NewServer(
 		opt(s)
 	}
 
-	// Moved here because we have a dependency on s.restClientCache
-	s.ghProviders = service.NewGithubProviderService(
-		store, eng, mt, provMt, &cfg.Provider, s.makeProjectForGitHubApp, s.restClientCache, s.fallbackTokenClient)
-
-	githubProviderManager := ghmanager.NewGitHubProviderClassManager(
-		s.restClientCache,
-		ghClientFactory,
-		&cfg.Provider,
-		fallbackTokenClient,
-		eng,
-		whManager,
-		store,
-		s.ghProviders,
-	)
-	providerManager, err := manager.NewProviderManager(providerStore, githubProviderManager)
-	if err != nil {
-		return nil, fmt.Errorf("failed to create provider manager: %w", err)
-	}
-	s.providerManager = providerManager
-	s.repos = github.NewRepositoryService(whManager, store, evt, providerManager)
+	// Finally set the ProjectFactory
+	// TODO: Get rid more of the override functions (specifically for the
+	// authz client) so that we can get rid of this hack.
+	projectFactoryWrapper.Factory = s.makeProjectForGitHubApp
 
 	return s, nil
 }

internal/controlplane/server_test.go

@@ -33,8 +33,12 @@ import (
 	mockdb "github.com/stacklok/minder/database/mock"
 	mockjwt "github.com/stacklok/minder/internal/auth/mock"
 	serverconfig "github.com/stacklok/minder/internal/config/server"
+	"github.com/stacklok/minder/internal/controlplane/metrics"
+	"github.com/stacklok/minder/internal/crypto"
 	"github.com/stacklok/minder/internal/events"
 	"github.com/stacklok/minder/internal/providers"
+	"github.com/stacklok/minder/internal/providers/github/clients"
+	ghService "github.com/stacklok/minder/internal/providers/github/service"
 	pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
 )
 
@@ -64,7 +68,11 @@ func init() {
 	// It would be nice if we could Close() the httpServer, but we leak it in the test instead
 }
 
-func newDefaultServer(t *testing.T, mockStore *mockdb.MockStore, opts ...ServerOption) (*Server, events.Interface) {
+func newDefaultServer(
+	t *testing.T,
+	mockStore *mockdb.MockStore,
+	clientFactory clients.GitHubClientFactory,
+) (*Server, events.Interface) {
 	t.Helper()
 
 	evt, err := events.Setup(context.Background(), &serverconfig.EventConfig{
@@ -84,7 +92,29 @@ func newDefaultServer(t *testing.T, mockStore *mockdb.MockStore, opts ...ServerO
 	defer ctrl.Finish()
 	mockJwt := mockjwt.NewMockJwtValidator(ctrl)
 
-	server, err := NewServer(mockStore, evt, c, mockJwt, providers.NewProviderStore(mockStore), opts...)
+	// Ugly, but needed to keep these tests working as is
+	// in future, beef up unit test coverage in the dependencies
+	// of this code, and refactor these tests to use stubs.
+	eng, err := crypto.EngineFromAuthConfig(&c.Auth)
+	require.NoError(t, err)
+	ghClientService := ghService.NewGithubProviderService(
+		mockStore,
+		eng,
+		metrics.NewNoopMetrics(),
+		nil,
+		nil,
+		clientFactory,
+	)
+
+	server, err := NewServer(
+		context.Background(),
+		mockStore,
+		evt,
+		c,
+		mockJwt,
+		providers.NewProviderStore(mockStore),
+		WithGitHubProviderService(ghClientService),
+	)
 	require.NoError(t, err, "failed to create server")
 	return server, evt
 }