From 4b7c75fcaf95ba6410c18ffb31ea3c461d2d4143 Mon Sep 17 00:00:00 2001 From: Don Browne Date: Tue, 30 Apr 2024 15:36:35 +0100 Subject: [PATCH] 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 | 2 +- internal/controlplane/common.go | 4 +- internal/controlplane/handlers_oauth_test.go | 19 +-- .../handlers_repositories_test.go | 2 +- internal/controlplane/handlers_user_test.go | 1 + internal/controlplane/server_test.go | 8 +- internal/engine/executor.go | 4 +- internal/projects/creator.go | 6 +- .../providers/github/{app => clients}/app.go | 71 ++++++----- .../github/{app => clients}/app_test.go | 21 ++-- internal/providers/github/clients/factory.go | 68 +++++++++- .../providers/github/clients/mock/factory.go | 74 +++++++++++ .../github/{oauth => clients}/oauth.go | 43 ++++--- .../github/{oauth => clients}/oauth_test.go | 17 ++- internal/providers/github/manager/manager.go | 12 +- internal/providers/github/service/service.go | 116 +++++++++--------- .../providers/github/service/service_test.go | 67 ++++++---- internal/providers/provider_definitions.go | 13 +- internal/providers/providers.go | 16 ++- internal/service/service.go | 4 +- 20 files changed, 371 insertions(+), 197 deletions(-) rename internal/providers/github/{app => clients}/app.go (79%) rename internal/providers/github/{app => clients}/app_test.go (94%) create mode 100644 internal/providers/github/clients/mock/factory.go rename internal/providers/github/{oauth => clients}/oauth.go (85%) rename internal/providers/github/{oauth => clients}/oauth_test.go (93%) diff --git a/cmd/cli/app/quickstart/quickstart.go b/cmd/cli/app/quickstart/quickstart.go index bc283162fc..0528da44d9 100644 --- a/cmd/cli/app/quickstart/quickstart.go +++ b/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" ) diff --git a/internal/controlplane/common.go b/internal/controlplane/common.go index 90537d9f6c..2c1771684a 100644 --- a/internal/controlplane/common.go +++ b/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" ) diff --git a/internal/controlplane/handlers_oauth_test.go b/internal/controlplane/handlers_oauth_test.go index ad79d433d7..23e0eb5a94 100644 --- a/internal/controlplane/handlers_oauth_test.go +++ b/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) diff --git a/internal/controlplane/handlers_repositories_test.go b/internal/controlplane/handlers_repositories_test.go index 38fd88f151..51781a5c87 100644 --- a/internal/controlplane/handlers_repositories_test.go +++ b/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" diff --git a/internal/controlplane/handlers_user_test.go b/internal/controlplane/handlers_user_test.go index 7c2f453451..c8be77869e 100644 --- a/internal/controlplane/handlers_user_test.go +++ b/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, diff --git a/internal/controlplane/server_test.go b/internal/controlplane/server_test.go index ed80411b6a..832af00749 100644 --- a/internal/controlplane/server_test.go +++ b/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{ diff --git a/internal/engine/executor.go b/internal/engine/executor.go index 6906c899b1..b4b78ab3ba 100644 --- a/internal/engine/executor.go +++ b/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 { diff --git a/internal/projects/creator.go b/internal/projects/creator.go index fd4e5c616d..df17c15923 100644 --- a/internal/projects/creator.go +++ b/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) diff --git a/internal/providers/github/app/app.go b/internal/providers/github/clients/app.go similarity index 79% rename from internal/providers/github/app/app.go rename to internal/providers/github/clients/app.go index fadb0d5954..59a5a1f039 100644 --- a/internal/providers/github/app/app.go +++ b/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,23 +84,21 @@ 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( @@ -91,12 +106,12 @@ func NewGitHubAppProvider( // 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 } diff --git a/internal/providers/github/app/app_test.go b/internal/providers/github/clients/app_test.go similarity index 94% rename from internal/providers/github/app/app_test.go rename to internal/providers/github/clients/app_test.go index 0c6f7f1d04..8d3367aa19 100644 --- a/internal/providers/github/app/app_test.go +++ b/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) diff --git a/internal/providers/github/clients/factory.go b/internal/providers/github/clients/factory.go index e7f03918cb..be583e291f 100644 --- a/internal/providers/github/clients/factory.go +++ b/internal/providers/github/clients/factory.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package clients contains the GitHub client factory +// Package clients contains github client logic package clients import ( @@ -27,16 +27,38 @@ import ( "golang.org/x/oauth2" "github.com/stacklok/minder/internal/db" + "github.com/stacklok/minder/internal/providers/github" "github.com/stacklok/minder/internal/providers/telemetry" provifv1 "github.com/stacklok/minder/pkg/providers/v1" ) +//go:generate go run go.uber.org/mock/mockgen -package mock_$GOPACKAGE -destination=./mock/$GOFILE -source=./$GOFILE + +// I don't particularly love having a factory which returns two types of thing, +// even if they are closely related. I think this can be cleaned up, but I +// think the right time to do it is when we get rid of the Github-specific +// provider trait. + // GitHubClientFactory creates instances of the GitHub API client type GitHubClientFactory interface { - // Build creates an instance of the GitHub Client + // BuildOAuthClient creates an instance of the GitHub Client and the OAuthDelegate + // `baseURL` should be set to the empty string if there is no need to + // override the default GitHub URL + BuildOAuthClient( + baseURL string, + credential provifv1.GitHubCredential, + owner string, + ) (*gogithub.Client, github.Delegate, error) + // BuildAppClient creates an instance of the GitHub Client and the AppDelegate // `baseURL` should be set to the empty string if there is no need to // override the default GitHub URL - Build(baseURL string, credential provifv1.GitHubCredential) (*gogithub.Client, error) + BuildAppClient( + baseURL string, + credential provifv1.GitHubCredential, + appName string, + userID int64, + isOrg bool, + ) (*gogithub.Client, github.Delegate, error) } type githubClientFactory struct { @@ -48,7 +70,43 @@ func NewGitHubClientFactory(metrics telemetry.HttpClientMetrics) GitHubClientFac return &githubClientFactory{metrics: metrics} } -func (g *githubClientFactory) Build(baseURL string, credential provifv1.GitHubCredential) (*gogithub.Client, error) { +func (g *githubClientFactory) BuildOAuthClient( + baseURL string, + credential provifv1.GitHubCredential, + owner string, +) (*gogithub.Client, github.Delegate, error) { + ghClient, err := g.buildClient(baseURL, credential) + if err != nil { + return nil, nil, err + } + return ghClient, NewOAuthDelegate(ghClient, credential, owner), nil +} + +func (g *githubClientFactory) BuildAppClient( + baseURL string, + credential provifv1.GitHubCredential, + appName string, + userID int64, + isOrg bool, +) (*gogithub.Client, github.Delegate, error) { + ghClient, err := g.buildClient(baseURL, credential) + if err != nil { + return nil, nil, err + } + delegate := NewAppDelegate( + ghClient, + credential, + appName, + userID, + isOrg, + ) + return ghClient, delegate, nil +} + +func (g *githubClientFactory) buildClient( + baseURL string, + credential provifv1.GitHubCredential, +) (*gogithub.Client, error) { tc := &http.Client{ Transport: &oauth2.Transport{ Base: http.DefaultClient.Transport, @@ -93,7 +151,7 @@ func (g *githubClientFactory) Build(baseURL string, credential provifv1.GitHubCr if baseURL != "" { parsedURL, err := url.Parse(baseURL) if err != nil { - return nil, err + return nil, fmt.Errorf("error parsing URL: %w", err) } ghClient.BaseURL = parsedURL } diff --git a/internal/providers/github/clients/mock/factory.go b/internal/providers/github/clients/mock/factory.go new file mode 100644 index 0000000000..e89268070c --- /dev/null +++ b/internal/providers/github/clients/mock/factory.go @@ -0,0 +1,74 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: ./factory.go +// +// Generated by this command: +// +// mockgen -package mock_clients -destination=./mock/factory.go -source=./factory.go +// + +// Package mock_clients is a generated GoMock package. +package mock_clients + +import ( + reflect "reflect" + + github "github.com/google/go-github/v61/github" + github0 "github.com/stacklok/minder/internal/providers/github" + v1 "github.com/stacklok/minder/pkg/providers/v1" + gomock "go.uber.org/mock/gomock" +) + +// MockGitHubClientFactory is a mock of GitHubClientFactory interface. +type MockGitHubClientFactory struct { + ctrl *gomock.Controller + recorder *MockGitHubClientFactoryMockRecorder +} + +// MockGitHubClientFactoryMockRecorder is the mock recorder for MockGitHubClientFactory. +type MockGitHubClientFactoryMockRecorder struct { + mock *MockGitHubClientFactory +} + +// NewMockGitHubClientFactory creates a new mock instance. +func NewMockGitHubClientFactory(ctrl *gomock.Controller) *MockGitHubClientFactory { + mock := &MockGitHubClientFactory{ctrl: ctrl} + mock.recorder = &MockGitHubClientFactoryMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockGitHubClientFactory) EXPECT() *MockGitHubClientFactoryMockRecorder { + return m.recorder +} + +// BuildAppClient mocks base method. +func (m *MockGitHubClientFactory) BuildAppClient(baseURL string, credential v1.GitHubCredential, appName string, userID int64, isOrg bool) (*github.Client, github0.Delegate, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "BuildAppClient", baseURL, credential, appName, userID, isOrg) + ret0, _ := ret[0].(*github.Client) + ret1, _ := ret[1].(github0.Delegate) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// BuildAppClient indicates an expected call of BuildAppClient. +func (mr *MockGitHubClientFactoryMockRecorder) BuildAppClient(baseURL, credential, appName, userID, isOrg any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BuildAppClient", reflect.TypeOf((*MockGitHubClientFactory)(nil).BuildAppClient), baseURL, credential, appName, userID, isOrg) +} + +// BuildOAuthClient mocks base method. +func (m *MockGitHubClientFactory) BuildOAuthClient(baseURL string, credential v1.GitHubCredential, owner string) (*github.Client, github0.Delegate, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "BuildOAuthClient", baseURL, credential, owner) + ret0, _ := ret[0].(*github.Client) + ret1, _ := ret[1].(github0.Delegate) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// BuildOAuthClient indicates an expected call of BuildOAuthClient. +func (mr *MockGitHubClientFactoryMockRecorder) BuildOAuthClient(baseURL, credential, owner any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BuildOAuthClient", reflect.TypeOf((*MockGitHubClientFactory)(nil).BuildOAuthClient), baseURL, credential, owner) +} diff --git a/internal/providers/github/oauth/oauth.go b/internal/providers/github/clients/oauth.go similarity index 85% rename from internal/providers/github/oauth/oauth.go rename to internal/providers/github/clients/oauth.go index e51de252c4..1862f8be7d 100644 --- a/internal/providers/github/oauth/oauth.go +++ b/internal/providers/github/clients/oauth.go @@ -12,8 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package oauth provides a client for interacting with the GitHub API using OAuth 2.0 authorization -package oauth +package clients import ( "context" @@ -24,7 +23,6 @@ import ( "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" @@ -34,16 +32,16 @@ import ( // Github is the string that represents the GitHubOAuth provider const Github = "github" -// Implements is the list of provider types that the GitHubOAuth provider implements -var Implements = []db.ProviderType{ +// OAuthImplements is the list of provider types that the GitHubOAuth provider implements +var OAuthImplements = []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{ +// OAuthAuthorizationFlows is the list of authorization flows that the GitHubOAuth provider supports +var OAuthAuthorizationFlows = []db.AuthorizationFlow{ db.AuthorizationFlowUserInput, db.AuthorizationFlowOauth2AuthorizationCodeFlow, } @@ -58,6 +56,21 @@ type GitHubOAuthDelegate struct { // Ensure that the GitHubOAuthDelegate client implements the GitHub Delegate interface var _ github.Delegate = (*GitHubOAuthDelegate)(nil) +// NewOAuthDelegate 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 NewOAuthDelegate( + client *gogithub.Client, + credential provifv1.GitHubCredential, + owner string, +) *GitHubOAuthDelegate { + return &GitHubOAuthDelegate{ + client: client, + credential: credential, + owner: owner, + } +} + // NewRestClient creates a new GitHub REST API client // BaseURL defaults to the public GitHub API, if needing to use a customer domain // endpoint (as is the case with GitHub Enterprise), set the Endpoint field in @@ -66,30 +79,24 @@ func NewRestClient( cfg *minderv1.GitHubProviderConfig, restClientCache ratecache.RestClientCache, credential provifv1.GitHubCredential, - ghClientFactory clients.GitHubClientFactory, + ghClientFactory GitHubClientFactory, owner string, ) (*github.GitHub, error) { - ghClient, err := ghClientFactory.Build(cfg.Endpoint, credential) + ghClient, delegate, err := ghClientFactory.BuildOAuthClient(cfg.Endpoint, credential, owner) if err != nil { return nil, err } - oauthDelegate := &GitHubOAuthDelegate{ - client: ghClient, - credential: credential, - owner: owner, - } - return github.NewGitHub( ghClient, ghClient, // use the same client for listing packages and all other operations restClientCache, - oauthDelegate, + delegate, ), nil } -// ParseV1Config parses the raw config into a GitHubConfig struct -func ParseV1Config(rawCfg json.RawMessage) (*minderv1.GitHubProviderConfig, error) { +// ParseV1OAuthConfig parses the raw config into a GitHubConfig struct +func ParseV1OAuthConfig(rawCfg json.RawMessage) (*minderv1.GitHubProviderConfig, error) { type wrapper struct { GitHub *minderv1.GitHubProviderConfig `json:"github" yaml:"github" mapstructure:"github" validate:"required"` } diff --git a/internal/providers/github/oauth/oauth_test.go b/internal/providers/github/clients/oauth_test.go similarity index 93% rename from internal/providers/github/oauth/oauth_test.go rename to internal/providers/github/clients/oauth_test.go index c9e8db7b8f..153244a01e 100644 --- a/internal/providers/github/oauth/oauth_test.go +++ b/internal/providers/github/clients/oauth_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package oauth +package clients import ( "context" @@ -30,7 +30,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" @@ -45,7 +44,7 @@ func TestNewRestClient(t *testing.T) { }, nil, credentials.NewGitHubTokenCredential("token"), - clients.NewGitHubClientFactory(provtelemetry.NewNoopMetrics()), + NewGitHubClientFactory(provtelemetry.NewNoopMetrics()), "", ) @@ -53,7 +52,7 @@ func TestNewRestClient(t *testing.T) { assert.NotNil(t, client) } -func TestArtifactAPIEscapes(t *testing.T) { +func TestArtifactAPIEscapesOAuth(t *testing.T) { t.Parallel() tests := []struct { @@ -121,7 +120,7 @@ func TestArtifactAPIEscapes(t *testing.T) { &minderv1.GitHubProviderConfig{Endpoint: testServer.URL + "/"}, nil, credentials.NewGitHubTokenCredential("token"), - clients.NewGitHubClientFactory(provtelemetry.NewNoopMetrics()), + NewGitHubClientFactory(provtelemetry.NewNoopMetrics()), "stacklok", ) assert.NoError(t, err) @@ -133,7 +132,7 @@ func TestArtifactAPIEscapes(t *testing.T) { } -func TestWaitForRateLimitReset(t *testing.T) { +func TestWaitForRateLimitResetOAuth(t *testing.T) { t.Parallel() token := "mockToken-2" @@ -155,7 +154,7 @@ func TestWaitForRateLimitReset(t *testing.T) { &minderv1.GitHubProviderConfig{Endpoint: server.URL + "/"}, ratecache.NewRestClientCache(context.Background()), credentials.NewGitHubTokenCredential(token), - clients.NewGitHubClientFactory(provtelemetry.NewNoopMetrics()), + NewGitHubClientFactory(provtelemetry.NewNoopMetrics()), "mockOwner", ) require.NoError(t, err) @@ -168,7 +167,7 @@ func TestWaitForRateLimitReset(t *testing.T) { assert.Equal(t, expectedReq, reqCount) } -func TestConcurrentWaitForRateLimitReset(t *testing.T) { +func TestConcurrentWaitForRateLimitResetOAuth(t *testing.T) { t.Parallel() restClientCache := ratecache.NewRestClientCache(context.Background()) @@ -205,7 +204,7 @@ func TestConcurrentWaitForRateLimitReset(t *testing.T) { &minderv1.GitHubProviderConfig{Endpoint: server.URL + "/"}, restClientCache, credentials.NewGitHubTokenCredential(token), - clients.NewGitHubClientFactory(provtelemetry.NewNoopMetrics()), + NewGitHubClientFactory(provtelemetry.NewNoopMetrics()), owner, ) require.NoError(t, err) diff --git a/internal/providers/github/manager/manager.go b/internal/providers/github/manager/manager.go index 74c264ce4b..2ce15c40f0 100644 --- a/internal/providers/github/manager/manager.go +++ b/internal/providers/github/manager/manager.go @@ -31,9 +31,7 @@ import ( "github.com/stacklok/minder/internal/db" "github.com/stacklok/minder/internal/providers" "github.com/stacklok/minder/internal/providers/credentials" - githubapp "github.com/stacklok/minder/internal/providers/github/app" "github.com/stacklok/minder/internal/providers/github/clients" - ghclient "github.com/stacklok/minder/internal/providers/github/oauth" "github.com/stacklok/minder/internal/providers/github/service" m "github.com/stacklok/minder/internal/providers/manager" "github.com/stacklok/minder/internal/providers/ratecache" @@ -111,12 +109,12 @@ func (g *githubProviderManager) Build(ctx context.Context, config *db.Provider) // previously this was done by checking the name, I think this is safer if class == db.ProviderClassGithub { // TODO: Parsing will change based on version - cfg, err := ghclient.ParseV1Config(config.Definition) + cfg, err := clients.ParseV1OAuthConfig(config.Definition) if err != nil { return nil, fmt.Errorf("error parsing github config: %w", err) } - cli, err := ghclient.NewRestClient( + cli, err := clients.NewRestClient( cfg, g.restClientCache, creds.credential, @@ -129,12 +127,12 @@ func (g *githubProviderManager) Build(ctx context.Context, config *db.Provider) return cli, nil } - cfg, err := githubapp.ParseV1Config(config.Definition) + cfg, err := clients.ParseV1AppConfig(config.Definition) if err != nil { return nil, fmt.Errorf("error parsing github app config: %w", err) } - cli, err := githubapp.NewGitHubAppProvider( + cli, err := clients.NewGitHubAppProvider( cfg, g.config.GitHubApp, g.restClientCache, @@ -249,7 +247,7 @@ func (g *githubProviderManager) createProviderWithInstallationToken( return nil, fmt.Errorf("error getting installation ID: %w", err) } - cfg, err := githubapp.ParseV1Config(prov.Definition) + cfg, err := clients.ParseV1AppConfig(prov.Definition) if err != nil { return nil, fmt.Errorf("error parsing github app config: %w", err) } diff --git a/internal/providers/github/service/service.go b/internal/providers/github/service/service.go index 5e143aa8c5..42669edf43 100644 --- a/internal/providers/github/service/service.go +++ b/internal/providers/github/service/service.go @@ -13,7 +13,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package service contains the github +// Package service contains the GitHubProviderService package service import ( @@ -41,9 +41,7 @@ import ( "github.com/stacklok/minder/internal/providers" "github.com/stacklok/minder/internal/providers/credentials" ghprov "github.com/stacklok/minder/internal/providers/github" - "github.com/stacklok/minder/internal/providers/github/app" - "github.com/stacklok/minder/internal/providers/ratecache" - provtelemetry "github.com/stacklok/minder/internal/providers/telemetry" + "github.com/stacklok/minder/internal/providers/github/clients" ) //go:generate go run go.uber.org/mock/mockgen -package mock_$GOPACKAGE -destination=./mock/$GOFILE -source=./$GOFILE @@ -86,15 +84,13 @@ type ProjectFactory func( ctx context.Context, qtx db.Querier, name string, user int64) (*db.Project, error) type ghProviderService struct { - store db.Store - cryptoEngine crypto.Engine - mt metrics.Metrics - provMt provtelemetry.ProviderMetrics - config *server.ProviderConfig - projectFactory ProjectFactory - restClientCache ratecache.RestClientCache - ghClientService ghprov.ClientService - fallbackTokenClient *github.Client + store db.Store + cryptoEngine crypto.Engine + mt metrics.Metrics + config *server.ProviderConfig + projectFactory ProjectFactory + ghClientService ghprov.ClientService + ghClientFactory clients.GitHubClientFactory } // NewGithubProviderService creates an instance of GitHubProviderService @@ -102,22 +98,18 @@ func NewGithubProviderService( store db.Store, cryptoEngine crypto.Engine, mt metrics.Metrics, - provMt provtelemetry.ProviderMetrics, config *server.ProviderConfig, projectFactory ProjectFactory, - restClientCache ratecache.RestClientCache, - fallbackTokenClient *github.Client, + ghClientFactory clients.GitHubClientFactory, ) GitHubProviderService { return &ghProviderService{ - store: store, - cryptoEngine: cryptoEngine, - mt: mt, - provMt: provMt, - config: config, - projectFactory: projectFactory, - restClientCache: restClientCache, - ghClientService: ghprov.ClientServiceImplementation{}, - fallbackTokenClient: fallbackTokenClient, + store: store, + cryptoEngine: cryptoEngine, + mt: mt, + config: config, + projectFactory: projectFactory, + ghClientService: ghprov.ClientServiceImplementation{}, + ghClientFactory: ghClientFactory, } } @@ -170,7 +162,13 @@ func (p *ghProviderService) CreateGitHubOAuthProvider( // Older enrollments may not have a RemoteUser stored; these should age out fairly quickly. p.mt.AddTokenOpCount(ctx, "check", stateData.RemoteUser.Valid) if stateData.RemoteUser.Valid { - if err := p.verifyProviderTokenIdentity(ctx, stateData, provider, token.AccessToken); err != nil { + credential := credentials.NewGitHubTokenCredential(token.AccessToken) + // owner is empty, as per original logic + _, delegate, err := p.ghClientFactory.BuildOAuthClient("", credential, "") + if err != nil { + return nil, fmt.Errorf("unable to create github client: %w", err) + } + if err := verifyProviderTokenIdentity(ctx, stateData, delegate); err != nil { return nil, ErrInvalidTokenIdentity } } else { @@ -231,11 +229,23 @@ func (p *ghProviderService) CreateGitHubAppProvider( } return db.WithTransaction(p.store, func(qtx db.ExtendQuerier) (*db.Provider, error) { - validateOwnership := func(ctx context.Context, provider db.Provider) error { + validateOwnership := func(ctx context.Context) error { // Older enrollments may not have a RemoteUser stored; these should age out fairly quickly. p.mt.AddTokenOpCount(ctx, "check", stateData.RemoteUser.Valid) if stateData.RemoteUser.Valid { - if err := p.verifyProviderTokenIdentity(ctx, stateData, provider, token.AccessToken); err != nil { + // create just enough of the provider to validate the user ID + credential := credentials.NewGitHubTokenCredential(token.AccessToken) + _, delegate, err := p.ghClientFactory.BuildAppClient( + "", + credential, + p.config.GitHubApp.AppName, + p.config.GitHubApp.UserID, + false, // isOrg = false, as per original logic + ) + if err != nil { + return fmt.Errorf("unable to create github client: %w", err) + } + if err := verifyProviderTokenIdentity(ctx, stateData, delegate); err != nil { return ErrInvalidTokenIdentity } } else { @@ -323,17 +333,29 @@ func createGitHubApp( projectId uuid.UUID, installationOwner *github.User, installationID int64, - validateOwnership func(ctx context.Context, provider db.Provider) error, + validateOwnership func(ctx context.Context) error, nonce sql.NullString, ) (db.Provider, error) { + if validateOwnership != nil { + if err := validateOwnership(ctx); err != nil { + return db.Provider{}, err + } + } + + class := db.ProviderClassGithubApp + providerDef, err := providers.GetProviderClassDefinition(string(class)) + if err != nil { + return db.Provider{}, err + } + // Save the installation ID and create a provider savedProvider, err := qtx.CreateProvider(ctx, db.CreateProviderParams{ Name: fmt.Sprintf("%s-%s", db.ProviderClassGithubApp, installationOwner.GetLogin()), ProjectID: projectId, - Class: db.ProviderClassGithubApp, - Implements: app.Implements, + Class: class, + Implements: providerDef.Traits, Definition: json.RawMessage(`{"github-app": {}}`), - AuthFlows: app.AuthorizationFlows, + AuthFlows: providerDef.AuthorizationFlows, }) if err != nil { return db.Provider{}, err @@ -341,15 +363,6 @@ func createGitHubApp( isOrg := installationOwner.GetType() == TypeGitHubOrganization - if validateOwnership != nil { - // TODO: it would be nice if validateOwnership didn't need to use a provider to get a - // github.Client, because then we could call it _before_ createGitHubApp, rather than - // in the middle... - if err := validateOwnership(ctx, savedProvider); err != nil { - return db.Provider{}, err - } - } - _, err = qtx.UpsertInstallationID(ctx, db.UpsertInstallationIDParams{ ProviderID: uuid.NullUUID{ UUID: savedProvider.ID, @@ -466,21 +479,12 @@ func (p *ghProviderService) DeleteInstallation(ctx context.Context, providerID u return nil } -func (p *ghProviderService) verifyProviderTokenIdentity( - ctx context.Context, stateData db.GetProjectIDBySessionStateRow, provider db.Provider, token string) error { - pbOpts := []providers.ProviderBuilderOption{ - providers.WithProviderMetrics(p.provMt), - providers.WithRestClientCache(p.restClientCache), - } - builder := providers.NewProviderBuilder(&provider, sql.NullString{}, false, credentials.NewGitHubTokenCredential(token), - p.config, p.fallbackTokenClient, pbOpts...) - // NOTE: this is github-specific at the moment. We probably need to generally - // re-think token enrollment when we add more providers. - ghClient, err := builder.GetGitHub() - if err != nil { - return fmt.Errorf("error creating GitHub client: %w", err) - } - userId, err := ghClient.GetUserId(ctx) +func verifyProviderTokenIdentity( + ctx context.Context, + stateData db.GetProjectIDBySessionStateRow, + client ghprov.Delegate, +) error { + userId, err := client.GetUserId(ctx) if err != nil { return fmt.Errorf("error getting user ID: %w", err) } diff --git a/internal/providers/github/service/service_test.go b/internal/providers/github/service/service_test.go index 91359c05c6..af115790e6 100644 --- a/internal/providers/github/service/service_test.go +++ b/internal/providers/github/service/service_test.go @@ -45,10 +45,9 @@ 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/app" + "github.com/stacklok/minder/internal/providers/github/clients" + mockclients "github.com/stacklok/minder/internal/providers/github/clients/mock" mockgh "github.com/stacklok/minder/internal/providers/github/mock" - ghclient "github.com/stacklok/minder/internal/providers/github/oauth" - mockratecache "github.com/stacklok/minder/internal/providers/ratecache/mock" "github.com/stacklok/minder/internal/providers/telemetry" "github.com/stacklok/minder/internal/util/rand" ) @@ -60,11 +59,12 @@ type testMocks struct { cancelFunc embedded.CancelFunc } -func testNewProviderService( +func testNewGitHubProviderService( t *testing.T, mockCtrl *gomock.Controller, config *server.ProviderConfig, projectFactory ProjectFactory, + ghClientFactory clients.GitHubClientFactory, ) (*ghProviderService, *testMocks) { t.Helper() @@ -89,15 +89,17 @@ func testNewProviderService( require.NoError(t, err) packageListingClient.BaseURL = testServerUrl + if ghClientFactory == nil { + ghClientFactory = clients.NewGitHubClientFactory(telemetry.NewNoopMetrics()) + } + psi := NewGithubProviderService( mocks.fakeStore, mocks.cryptoMocks, metrics.NewNoopMetrics(), - telemetry.NewNoopMetrics(), config, projectFactory, - mockratecache.NewMockRestClientCache(mockCtrl), - packageListingClient, + ghClientFactory, ) ps, ok := psi.(*ghProviderService) @@ -130,13 +132,23 @@ func TestProviderService_CreateGitHubOAuthProvider(t *testing.T) { const ( stateNonce = "test-oauth-nonce" stateNonceUpdate = "test-oauth-nonce-update" + accountID = 12345 ) ctrl := gomock.NewController(t) defer ctrl.Finish() cfg := &server.ProviderConfig{} - provSvc, mocks := testNewProviderService(t, ctrl, cfg, nil) + delegate := mockgh.NewMockDelegate(ctrl) + delegate.EXPECT(). + GetUserId(gomock.Any()). + Return(int64(accountID), nil) + clientFactory := mockclients.NewMockGitHubClientFactory(ctrl) + clientFactory.EXPECT(). + BuildOAuthClient(gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, delegate, nil) + + provSvc, mocks := testNewGitHubProviderService(t, ctrl, cfg, nil, clientFactory) dbproj, err := mocks.fakeStore.CreateProject(context.Background(), db.CreateProjectParams{ Name: "test", @@ -150,7 +162,7 @@ func TestProviderService_CreateGitHubOAuthProvider(t *testing.T) { dbProv, err := provSvc.CreateGitHubOAuthProvider( context.Background(), - ghclient.Github, + clients.Github, db.ProviderClassGithub, oauth2.Token{ AccessToken: "my-access", @@ -163,15 +175,16 @@ func TestProviderService_CreateGitHubOAuthProvider(t *testing.T) { String: "testorg", }, RemoteUser: sql.NullString{ - Valid: false, + Valid: true, + String: strconv.Itoa(accountID), }, }, stateNonce) require.NoError(t, err) require.NotNil(t, dbProv) require.Equal(t, dbProv.ProjectID, dbproj.ID) - require.Equal(t, dbProv.AuthFlows, ghclient.AuthorizationFlows) - require.Equal(t, dbProv.Implements, ghclient.Implements) + require.Equal(t, dbProv.AuthFlows, clients.OAuthAuthorizationFlows) + require.Equal(t, dbProv.Implements, clients.OAuthImplements) dbToken, err := mocks.fakeStore.GetAccessTokenByProvider(context.Background(), dbProv.Name) require.NoError(t, err) @@ -187,7 +200,7 @@ func TestProviderService_CreateGitHubOAuthProvider(t *testing.T) { dbProvUpdated, err := provSvc.CreateGitHubOAuthProvider( context.Background(), - ghclient.Github, + clients.Github, db.ProviderClassGithub, oauth2.Token{ AccessToken: "my-access2", @@ -239,7 +252,16 @@ func TestProviderService_CreateGitHubAppProvider(t *testing.T) { }, } - provSvc, mocks := testNewProviderService(t, ctrl, cfg, nil) + delegate := mockgh.NewMockDelegate(ctrl) + delegate.EXPECT(). + GetUserId(gomock.Any()). + Return(int64(accountID), nil) + clientFactory := mockclients.NewMockGitHubClientFactory(ctrl) + clientFactory.EXPECT(). + BuildAppClient(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, delegate, nil) + + provSvc, mocks := testNewGitHubProviderService(t, ctrl, cfg, nil, clientFactory) dbproj, err := mocks.fakeStore.CreateProject(context.Background(), db.CreateProjectParams{ Name: "test", @@ -261,7 +283,8 @@ func TestProviderService_CreateGitHubAppProvider(t *testing.T) { db.GetProjectIDBySessionStateRow{ ProjectID: dbproj.ID, RemoteUser: sql.NullString{ - Valid: false, + Valid: true, + String: strconv.Itoa(accountID), }, }, installationID, @@ -270,8 +293,8 @@ func TestProviderService_CreateGitHubAppProvider(t *testing.T) { require.NotNil(t, dbProv) require.Equal(t, dbProv.ProjectID, dbproj.ID) - require.Equal(t, dbProv.AuthFlows, app.AuthorizationFlows) - require.Equal(t, dbProv.Implements, app.Implements) + require.Equal(t, dbProv.AuthFlows, clients.AppAuthorizationFlows) + require.Equal(t, dbProv.Implements, clients.AppImplements) require.Equal(t, dbProv.Class, db.ProviderClassGithubApp) require.Contains(t, dbProv.Name, db.ProviderClassGithubApp) require.Contains(t, dbProv.Name, accountLogin) @@ -319,7 +342,7 @@ func TestProviderService_CreateGitHubAppWithNewProject(t *testing.T) { return &project, nil } - provSvc, mocks := testNewProviderService(t, ctrl, cfg, factory) + provSvc, mocks := testNewGitHubProviderService(t, ctrl, cfg, factory, nil) mocks.svcMock.EXPECT(). GetInstallation(gomock.Any(), int64(installationID), gomock.Any()). @@ -376,7 +399,7 @@ func TestProviderService_CreateUnclaimedGitHubAppInstallation(t *testing.T) { return nil, errors.New("error getting user for GitHub ID: 404 not found") } - provSvc, mocks := testNewProviderService(t, ctrl, cfg, factory) + provSvc, mocks := testNewGitHubProviderService(t, ctrl, cfg, factory, nil) mocks.svcMock.EXPECT(). GetInstallation(gomock.Any(), int64(installationID), gomock.Any()). @@ -418,7 +441,7 @@ func TestProviderService_ValidateGithubInstallationId(t *testing.T) { }, } - provSvc, mocks := testNewProviderService(t, ctrl, cfg, nil) + provSvc, mocks := testNewGitHubProviderService(t, ctrl, cfg, nil, nil) mocks.svcMock.EXPECT(). ListUserInstallations(gomock.Any(), gomock.Any()). @@ -462,7 +485,7 @@ func TestProviderService_ValidateGitHubAppWebhookPayload(t *testing.T) { }, } - provSvc, _ := testNewProviderService(t, ctrl, cfg, nil) + provSvc, _ := testNewGitHubProviderService(t, ctrl, cfg, nil, nil) payload, err := provSvc.ValidateGitHubAppWebhookPayload(req) require.NoError(t, err) @@ -493,7 +516,7 @@ func TestProviderService_DeleteInstallation(t *testing.T) { }, } - provSvc, mocks := testNewProviderService(t, ctrl, cfg, nil) + provSvc, mocks := testNewGitHubProviderService(t, ctrl, cfg, nil, nil) dbproj, err := mocks.fakeStore.CreateProject(context.Background(), db.CreateProjectParams{ diff --git a/internal/providers/provider_definitions.go b/internal/providers/provider_definitions.go index 308e850f0f..c44aa61ac4 100644 --- a/internal/providers/provider_definitions.go +++ b/internal/providers/provider_definitions.go @@ -18,8 +18,7 @@ import ( "fmt" "github.com/stacklok/minder/internal/db" - githubapp "github.com/stacklok/minder/internal/providers/github/app" - ghclient "github.com/stacklok/minder/internal/providers/github/oauth" + ghclient "github.com/stacklok/minder/internal/providers/github/clients" ) // ProviderClassDefinition contains the static fields needed when creating a provider @@ -29,13 +28,13 @@ type ProviderClassDefinition struct { } var supportedProviderClassDefinitions = map[string]ProviderClassDefinition{ - githubapp.GithubApp: { - Traits: githubapp.Implements, - AuthorizationFlows: githubapp.AuthorizationFlows, + ghclient.GithubApp: { + Traits: ghclient.AppImplements, + AuthorizationFlows: ghclient.AppAuthorizationFlows, }, ghclient.Github: { - Traits: ghclient.Implements, - AuthorizationFlows: ghclient.AuthorizationFlows, + Traits: ghclient.OAuthImplements, + AuthorizationFlows: ghclient.OAuthAuthorizationFlows, }, } diff --git a/internal/providers/providers.go b/internal/providers/providers.go index 27b4ab9343..21f9434e8e 100644 --- a/internal/providers/providers.go +++ b/internal/providers/providers.go @@ -32,9 +32,7 @@ import ( "github.com/stacklok/minder/internal/db" "github.com/stacklok/minder/internal/providers/credentials" gitclient "github.com/stacklok/minder/internal/providers/git" - githubapp "github.com/stacklok/minder/internal/providers/github/app" "github.com/stacklok/minder/internal/providers/github/clients" - ghclient "github.com/stacklok/minder/internal/providers/github/oauth" httpclient "github.com/stacklok/minder/internal/providers/http" "github.com/stacklok/minder/internal/providers/ratecache" "github.com/stacklok/minder/internal/providers/telemetry" @@ -210,9 +208,9 @@ func (pb *ProviderBuilder) GetGitHub() (provinfv1.GitHub, error) { } // TODO: use provider class once it's available - if pb.p.Name == ghclient.Github { + if pb.p.Name == clients.Github { // TODO: Parsing will change based on version - cfg, err := ghclient.ParseV1Config(pb.p.Definition) + cfg, err := clients.ParseV1OAuthConfig(pb.p.Definition) if err != nil { return nil, fmt.Errorf("error parsing github config: %w", err) } @@ -220,14 +218,14 @@ func (pb *ProviderBuilder) GetGitHub() (provinfv1.GitHub, error) { // This should be passed in from the outside, but since I intend to // get rid of ProviderBuilder, I am instantiating a new copy here ghClientFactory := clients.NewGitHubClientFactory(pb.metrics) - cli, err := ghclient.NewRestClient(cfg, pb.restClientCache, gitHubCredential, ghClientFactory, pb.ownerFilter.String) + cli, err := clients.NewRestClient(cfg, pb.restClientCache, gitHubCredential, ghClientFactory, pb.ownerFilter.String) if err != nil { return nil, fmt.Errorf("error creating github client: %w", err) } return cli, nil } - cfg, err := githubapp.ParseV1Config(pb.p.Definition) + cfg, err := clients.ParseV1AppConfig(pb.p.Definition) if err != nil { return nil, fmt.Errorf("error parsing github app config: %w", err) } @@ -235,7 +233,7 @@ func (pb *ProviderBuilder) GetGitHub() (provinfv1.GitHub, error) { // get rid of ProviderBuilder, I am instantiating a new copy here ghClientFactory := clients.NewGitHubClientFactory(pb.metrics) - cli, err := githubapp.NewGitHubAppProvider(cfg, pb.cfg.GitHubApp, pb.restClientCache, gitHubCredential, + cli, err := clients.NewGitHubAppProvider(cfg, pb.cfg.GitHubApp, pb.restClientCache, gitHubCredential, pb.fallbackTokenClient, ghClientFactory, pb.isOrg) if err != nil { return nil, fmt.Errorf("error creating github app client: %w", err) @@ -370,7 +368,7 @@ func getInstallationTokenCredential( } else if err != nil { return nil, fmt.Errorf("error getting installation ID: %w", err) } - cfg, err := githubapp.ParseV1Config(prov.Definition) + cfg, err := clients.ParseV1AppConfig(prov.Definition) if err != nil { return nil, fmt.Errorf("error parsing github app config: %w", err) } @@ -430,7 +428,7 @@ func createProviderWithInstallationToken( return nil, fmt.Errorf("error getting installation ID: %w", err) } - cfg, err := githubapp.ParseV1Config(prov.Definition) + cfg, err := clients.ParseV1AppConfig(prov.Definition) if err != nil { return nil, fmt.Errorf("error parsing github app config: %w", err) } diff --git a/internal/service/service.go b/internal/service/service.go index 3d71660082..9985b7d3fa 100644 --- a/internal/service/service.go +++ b/internal/service/service.go @@ -97,11 +97,9 @@ func AllInOneServerService( store, cryptoEngine, serverMetrics, - providerMetrics, &cfg.Provider, makeProjectFactory(projectCreator, cfg.Identity), - restClientCache, - fallbackTokenClient, + ghClientFactory, ) githubProviderManager := ghmanager.NewGitHubProviderClassManager( restClientCache,