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

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_oauth_test.go

@@ -48,10 +48,10 @@ import (
 	"github.com/stacklok/minder/internal/engine"
 	"github.com/stacklok/minder/internal/events"
 	"github.com/stacklok/minder/internal/providers"
+	mockclients "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"
 )
@@ -405,16 +405,19 @@ func TestProviderCallback(t *testing.T) {
 			gh := mockgh.NewMockGitHub(ctrl)
 			gh.EXPECT().GetUserId(gomock.Any()).Return(int64(31337), nil).AnyTimes()
 
-			var restClientCache ratecache.RestClientCache
+			var clientFactory *mockclients.MockGitHubClientFactory
 			if tc.remoteUser.String != "" {
-				// TODO: verfifyProviderTokenIdentity
-				cancelable, cancel := context.WithCancel(context.Background())
-				defer cancel()
-				restClientCache = ratecache.NewRestClientCache(cancelable)
-				restClientCache.Set("", "anAccessToken", db.ProviderTypeGithub, gh)
+				delegate := mockgh.NewMockDelegate(ctrl)
+				delegate.EXPECT().
+					GetUserId(gomock.Any()).
+					Return(int64(31337), nil)
+				clientFactory = mockclients.NewMockGitHubClientFactory(ctrl)
+				clientFactory.EXPECT().
+					BuildOAuthClient(gomock.Any(), gomock.Any(), gomock.Any()).
+					Return(nil, delegate, nil)
 			}
 
-			s, _ := newDefaultServer(t, store, restClientCache)
+			s, _ := newDefaultServer(t, store, clientFactory)
 
 			var err error
 			encryptedUrlString, err := s.cryptoEngine.EncryptString(tc.redirectUrl)

internal/controlplane/handlers_repositories_test.go

@@ -30,8 +30,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/manager"
 	mockmanager "github.com/stacklok/minder/internal/providers/manager/mock"
 	ghrepo "github.com/stacklok/minder/internal/repositories/github"

internal/controlplane/handlers_user_test.go

@@ -453,6 +453,7 @@ func TestDeleteUser_gRPC(t *testing.T) {
 				GoChannel: serverconfig.GoChannelEventConfig{},
 			})
 			require.NoError(t, err, "failed to setup eventer")
+
 			server := &Server{
 				evt:           evt,
 				store:         mockStore,

internal/controlplane/server_test.go

@@ -39,8 +39,8 @@ import (
 	"github.com/stacklok/minder/internal/crypto"
 	"github.com/stacklok/minder/internal/events"
 	"github.com/stacklok/minder/internal/providers"
+	ghclient "github.com/stacklok/minder/internal/providers/github/clients"
 	ghService "github.com/stacklok/minder/internal/providers/github/service"
-	"github.com/stacklok/minder/internal/providers/ratecache"
 	pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
 )
 
@@ -73,7 +73,7 @@ func init() {
 func newDefaultServer(
 	t *testing.T,
 	mockStore *mockdb.MockStore,
-	restClientCache ratecache.RestClientCache,
+	ghClientFactory ghclient.GitHubClientFactory,
 ) (*Server, events.Interface) {
 	t.Helper()
 
@@ -106,9 +106,7 @@ func newDefaultServer(
 		// These nil dependencies do not matter for the current tests
 		nil,
 		nil,
-		nil,
-		restClientCache,
-		nil,
+		ghClientFactory,
 	)
 
 	server := &Server{

internal/engine/executor.go

@@ -94,7 +94,7 @@ func NewExecutor(
 	provCfg *serverconfig.ProviderConfig,
 	evt events.Publisher,
 	providerStore providers.ProviderStore,
-	restCacheClient ratecache.RestClientCache,
+	restClientCache ratecache.RestClientCache,
 	opts ...ExecutorOption,
 ) (*Executor, error) {
 	crypteng, err := crypto.EngineFromAuthConfig(authCfg)
@@ -115,7 +115,7 @@ func NewExecutor(
 		provCfg:                provCfg,
 		providerStore:          providerStore,
 		fallbackTokenClient:    fallbackTokenClient,
-		restClientCache:        restCacheClient,
+		restClientCache:        restClientCache,
 	}
 
 	for _, opt := range opts {

internal/projects/creator.go

@@ -27,7 +27,7 @@ import (
 	"github.com/stacklok/minder/internal/config/server"
 	"github.com/stacklok/minder/internal/db"
 	"github.com/stacklok/minder/internal/marketplaces"
-	github "github.com/stacklok/minder/internal/providers/github/oauth"
+	github "github.com/stacklok/minder/internal/providers/github/clients"
 	"github.com/stacklok/minder/pkg/mindpak"
 )
 
@@ -94,9 +94,9 @@ func (p *projectCreator) ProvisionSelfEnrolledOAuthProject(
 		Name:       github.Github,
 		ProjectID:  project.ID,
 		Class:      db.ProviderClassGithub,
-		Implements: github.Implements,
+		Implements: github.OAuthImplements,
 		Definition: json.RawMessage(`{"github": {}}`),
-		AuthFlows:  github.AuthorizationFlows,
+		AuthFlows:  github.OAuthAuthorizationFlows,
 	})
 	if err != nil {
 		return nil, fmt.Errorf("failed to create provider: %v", err)

internal/providers/github/app/app.go → internal/providers/github/clients/app.go

@@ -12,8 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-// Package app provides the GitHub App specific operations
-package app
+package clients
 
 import (
 	"context"
@@ -25,7 +24,6 @@ import (
 	"github.com/stacklok/minder/internal/config/server"
 	"github.com/stacklok/minder/internal/db"
 	"github.com/stacklok/minder/internal/providers/github"
-	"github.com/stacklok/minder/internal/providers/github/clients"
 	ghcommon "github.com/stacklok/minder/internal/providers/github/common"
 	"github.com/stacklok/minder/internal/providers/ratecache"
 	minderv1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
@@ -35,26 +33,45 @@ import (
 // GithubApp is the string that represents the GitHubApp provider
 const GithubApp = "github-app"
 
-// Implements is the list of provider types that the GitHubOAuth provider implements
-var Implements = []db.ProviderType{
+// AppImplements is the list of provider types that the GitHubOAuth provider implements
+var AppImplements = []db.ProviderType{
 	db.ProviderTypeGithub,
 	db.ProviderTypeGit,
 	db.ProviderTypeRest,
 	db.ProviderTypeRepoLister,
 }
 
-// AuthorizationFlows is the list of authorization flows that the GitHubOAuth provider supports
-var AuthorizationFlows = []db.AuthorizationFlow{
+// AppAuthorizationFlows is the list of authorization flows that the GitHubOAuth provider supports
+var AppAuthorizationFlows = []db.AuthorizationFlow{
 	db.AuthorizationFlowGithubAppFlow,
 }
 
 // GitHubAppDelegate is the struct that contains the GitHub App specific operations
 type GitHubAppDelegate struct {
-	client     *gogithub.Client
-	credential provifv1.GitHubCredential
-	appName    string
-	userId     int64
-	isOrg      bool
+	client        *gogithub.Client
+	credential    provifv1.GitHubCredential
+	appName       string
+	defaultUserId int64
+	isOrg         bool
+}
+
+// NewAppDelegate creates a GitHubOAuthDelegate from a GitHub client
+// This exists as a separate function to allow the provider creation code
+// to use its methods without instantiating a full provider.
+func NewAppDelegate(
+	client *gogithub.Client,
+	credential provifv1.GitHubCredential,
+	appName string,
+	defaultUserId int64,
+	isOrg bool,
+) *GitHubAppDelegate {
+	return &GitHubAppDelegate{
+		client:        client,
+		credential:    credential,
+		appName:       appName,
+		defaultUserId: defaultUserId,
+		isOrg:         isOrg,
+	}
 }
 
 // NewGitHubAppProvider creates a new GitHub App API client
@@ -67,36 +84,34 @@ func NewGitHubAppProvider(
 	restClientCache ratecache.RestClientCache,
 	credential provifv1.GitHubCredential,
 	packageListingClient *gogithub.Client,
-	ghClientFactory clients.GitHubClientFactory,
+	ghClientFactory GitHubClientFactory,
 	isOrg bool,
 ) (*github.GitHub, error) {
-	ghClient, err := ghClientFactory.Build(cfg.Endpoint, credential)
-	if err != nil {
-		return nil, err
-	}
-
 	appName := appConfig.AppName
 	userId := appConfig.UserID
 
-	oauthDelegate := &GitHubAppDelegate{
-		client:     ghClient,
-		credential: credential,
-		appName:    appName,
-		userId:     userId,
-		isOrg:      isOrg,
+	ghClient, delegate, err := ghClientFactory.BuildAppClient(
+		cfg.Endpoint,
+		credential,
+		appName,
+		userId,
+		isOrg,
+	)
+	if err != nil {
+		return nil, err
 	}
 
 	return github.NewGitHub(
 		ghClient,
 		// Use the fallback token for package listing, since fine-grained tokens don't have access
 		packageListingClient,
 		restClientCache,
-		oauthDelegate,
+		delegate,
 	), nil
 }
 
-// ParseV1Config parses the raw config into a GitHubAppProviderConfig struct
-func ParseV1Config(rawCfg json.RawMessage) (*minderv1.GitHubAppProviderConfig, error) {
+// ParseV1AppConfig parses the raw config into a GitHubAppProviderConfig struct
+func ParseV1AppConfig(rawCfg json.RawMessage) (*minderv1.GitHubAppProviderConfig, error) {
 	type wrapper struct {
 		GitHubApp *minderv1.GitHubAppProviderConfig `json:"github-app" yaml:"github-app" mapstructure:"github-app" validate:"required"`
 	}
@@ -168,7 +183,7 @@ func (g *GitHubAppDelegate) GetUserId(ctx context.Context) (int64, error) {
 	if err != nil {
 		// Fallback to the configured user ID
 		// note: this is different from the App ID
-		return g.userId, nil
+		return g.defaultUserId, nil
 	}
 	return user.GetID(), nil
 }

internal/providers/github/app/app_test.go → internal/providers/github/clients/app_test.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package app
+package clients
 
 import (
 	"context"
@@ -34,7 +34,6 @@ import (
 	"github.com/stacklok/minder/internal/db"
 	"github.com/stacklok/minder/internal/providers/credentials"
 	github2 "github.com/stacklok/minder/internal/providers/github"
-	"github.com/stacklok/minder/internal/providers/github/clients"
 	"github.com/stacklok/minder/internal/providers/ratecache"
 	provtelemetry "github.com/stacklok/minder/internal/providers/telemetry"
 	minderv1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
@@ -49,7 +48,7 @@ func TestNewGitHubAppProvider(t *testing.T) {
 		nil,
 		credentials.NewGitHubTokenCredential("token"),
 		github.NewClient(http.DefaultClient),
-		clients.NewGitHubClientFactory(provtelemetry.NewNoopMetrics()),
+		NewGitHubClientFactory(provtelemetry.NewNoopMetrics()),
 		false,
 	)
 
@@ -75,7 +74,7 @@ func TestUserInfo(t *testing.T) {
 		ratecache.NewRestClientCache(context.Background()),
 		credentials.NewGitHubTokenCredential("token"),
 		github.NewClient(http.DefaultClient),
-		clients.NewGitHubClientFactory(provtelemetry.NewNoopMetrics()),
+		NewGitHubClientFactory(provtelemetry.NewNoopMetrics()),
 		false,
 	)
 	assert.NoError(t, err)
@@ -97,7 +96,7 @@ func TestUserInfo(t *testing.T) {
 	assert.Equal(t, "123456789+github-actions[bot]@users.noreply.github.com", email)
 }
 
-func TestArtifactAPIEscapes(t *testing.T) {
+func TestArtifactAPIEscapesApp(t *testing.T) {
 	t.Parallel()
 
 	tests := []struct {
@@ -172,7 +171,7 @@ func TestArtifactAPIEscapes(t *testing.T) {
 				ratecache.NewRestClientCache(context.Background()),
 				credentials.NewGitHubTokenCredential("token"),
 				packageListingClient,
-				clients.NewGitHubClientFactory(provtelemetry.NewNoopMetrics()),
+				NewGitHubClientFactory(provtelemetry.NewNoopMetrics()),
 				true,
 			)
 
@@ -261,7 +260,7 @@ func TestListPackagesByRepository(t *testing.T) {
 				ratecache.NewRestClientCache(context.Background()),
 				credentials.NewGitHubTokenCredential(accessToken),
 				packageListingClient,
-				clients.NewGitHubClientFactory(provtelemetry.NewNoopMetrics()),
+				NewGitHubClientFactory(provtelemetry.NewNoopMetrics()),
 				true,
 			)
 			assert.NoError(t, err)
@@ -280,7 +279,7 @@ func TestListPackagesByRepository(t *testing.T) {
 	}
 }
 
-func TestWaitForRateLimitReset(t *testing.T) {
+func TestWaitForRateLimitResetApp(t *testing.T) {
 	t.Parallel()
 
 	token := "mockToken-2"
@@ -309,7 +308,7 @@ func TestWaitForRateLimitReset(t *testing.T) {
 		ratecache.NewRestClientCache(context.Background()),
 		credentials.NewGitHubTokenCredential(token),
 		packageListingClient,
-		clients.NewGitHubClientFactory(provtelemetry.NewNoopMetrics()),
+		NewGitHubClientFactory(provtelemetry.NewNoopMetrics()),
 		false,
 	)
 	require.NoError(t, err)
@@ -322,7 +321,7 @@ func TestWaitForRateLimitReset(t *testing.T) {
 	assert.Equal(t, expectedReq, reqCount)
 }
 
-func TestConcurrentWaitForRateLimitReset(t *testing.T) {
+func TestConcurrentWaitForRateLimitResetApp(t *testing.T) {
 	t.Parallel()
 
 	restClientCache := ratecache.NewRestClientCache(context.Background())
@@ -366,7 +365,7 @@ func TestConcurrentWaitForRateLimitReset(t *testing.T) {
 			restClientCache,
 			credentials.NewGitHubTokenCredential(token),
 			packageListingClient,
-			clients.NewGitHubClientFactory(provtelemetry.NewNoopMetrics()),
+			NewGitHubClientFactory(provtelemetry.NewNoopMetrics()),
 			false,
 		)
 		require.NoError(t, err)