diff --git a/cmd/frontend/internal/batches/resolvers/changeset_counts_test.go b/cmd/frontend/internal/batches/resolvers/changeset_counts_test.go index 77c18e75ec8..8c6f1a77ece 100644 --- a/cmd/frontend/internal/batches/resolvers/changeset_counts_test.go +++ b/cmd/frontend/internal/batches/resolvers/changeset_counts_test.go @@ -189,7 +189,9 @@ func TestChangesetCountsOverTimeIntegration(t *testing.T) { t.Fatal(err) } - src, err := sourcer.ForChangeset(ctx, bstore, c, sources.AuthenticationStrategyUserCredential, githubRepo, "") + src, err := sourcer.ForChangeset(ctx, bstore, c, githubRepo, sources.SourcerOpts{ + AuthenticationStrategy: sources.AuthenticationStrategyUserCredential, + }) if err != nil { t.Fatalf("failed to build source for repo: %s", err) } diff --git a/cmd/frontend/internal/batches/webhooks/bitbucketserver_test.go b/cmd/frontend/internal/batches/webhooks/bitbucketserver_test.go index 02552498963..1ab73a33b53 100644 --- a/cmd/frontend/internal/batches/webhooks/bitbucketserver_test.go +++ b/cmd/frontend/internal/batches/webhooks/bitbucketserver_test.go @@ -166,7 +166,9 @@ func testBitbucketServerWebhook(db database.DB, userID int32) func(*testing.T) { if err := s.CreateChangeset(ctx, ch); err != nil { t.Fatal(err) } - src, err := sourcer.ForChangeset(ctx, s, ch, sources.AuthenticationStrategyUserCredential, bitbucketRepo, "") + src, err := sourcer.ForChangeset(ctx, s, ch, bitbucketRepo, sources.SourcerOpts{ + AuthenticationStrategy: sources.AuthenticationStrategyUserCredential, + }) if err != nil { t.Fatal(err) } diff --git a/cmd/frontend/internal/batches/webhooks/github_test.go b/cmd/frontend/internal/batches/webhooks/github_test.go index 3221a7a6bdb..a751b830ad9 100644 --- a/cmd/frontend/internal/batches/webhooks/github_test.go +++ b/cmd/frontend/internal/batches/webhooks/github_test.go @@ -155,7 +155,9 @@ func testGitHubWebhook(db database.DB, userID int32) func(*testing.T) { VCS: protocol.VCSInfo{URL: "https://example.com/repo/"}, }) - src, err := sourcer.ForChangeset(ctx, s, changeset, sources.AuthenticationStrategyUserCredential, githubRepo, "") + src, err := sourcer.ForChangeset(ctx, s, changeset, githubRepo, sources.SourcerOpts{ + AuthenticationStrategy: sources.AuthenticationStrategyUserCredential, + }) if err != nil { t.Fatal(err) } diff --git a/internal/batches/reconciler/executor.go b/internal/batches/reconciler/executor.go index d3335222457..17c89fc3803 100644 --- a/internal/batches/reconciler/executor.go +++ b/internal/batches/reconciler/executor.go @@ -4,7 +4,6 @@ import ( "bytes" "context" "fmt" - ghtypes "github.com/sourcegraph/sourcegraph/internal/github_apps/types" "strings" "sync" "text/template" @@ -22,6 +21,7 @@ import ( "github.com/sourcegraph/sourcegraph/internal/conf" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/errcode" + ghtypes "github.com/sourcegraph/sourcegraph/internal/github_apps/types" "github.com/sourcegraph/sourcegraph/internal/gitserver" "github.com/sourcegraph/sourcegraph/internal/gitserver/protocol" "github.com/sourcegraph/sourcegraph/internal/repos" @@ -594,7 +594,9 @@ func (e *executor) decorateChangesetBody(ctx context.Context) (string, error) { } func loadChangesetSource(ctx context.Context, s *store.Store, sourcer sources.Sourcer, ch *btypes.Changeset, repo *types.Repo) (sources.ChangesetSource, error) { - css, err := sourcer.ForChangeset(ctx, s, ch, sources.AuthenticationStrategyUserCredential, repo, "") + css, err := sourcer.ForChangeset(ctx, s, ch, repo, sources.SourcerOpts{ + AuthenticationStrategy: sources.AuthenticationStrategyUserCredential, + }) if err != nil { switch err { case sources.ErrMissingCredentials: @@ -652,7 +654,11 @@ func (e *executor) runAfterCommit(ctx context.Context, css sources.ChangesetSour // configured for Batch Changes to sign commits on this code host with. if _, ok := css.(*sources.GitHubSource); ok { // Attempt to get a ChangesetSource authenticated with a GitHub App. - css, err = e.sourcer.ForChangeset(ctx, e.tx, e.ch, sources.AuthenticationStrategyGitHubApp, e.remote, ghtypes.SiteCredentialGitHubAppKind) + css, err = e.sourcer.ForChangeset(ctx, e.tx, e.ch, e.remote, sources.SourcerOpts{ + AuthenticationStrategy: sources.AuthenticationStrategyGitHubApp, + GitHubAppKind: ghtypes.CommitSigningGitHubAppKind, + AsNonCredential: true, + }) if err != nil { switch err { case sources.ErrNoGitHubAppConfigured: diff --git a/internal/batches/reconciler/executor_test.go b/internal/batches/reconciler/executor_test.go index 5e80f54c86d..beb0633c89c 100644 --- a/internal/batches/reconciler/executor_test.go +++ b/internal/batches/reconciler/executor_test.go @@ -14,9 +14,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/sourcegraph/sourcegraph/internal/gitserver" - "github.com/sourcegraph/sourcegraph/internal/httpcli" - "github.com/sourcegraph/log/logtest" "github.com/sourcegraph/sourcegraph/internal/actor" @@ -32,8 +29,10 @@ import ( et "github.com/sourcegraph/sourcegraph/internal/encryption/testing" "github.com/sourcegraph/sourcegraph/internal/errcode" "github.com/sourcegraph/sourcegraph/internal/extsvc/auth" + "github.com/sourcegraph/sourcegraph/internal/gitserver" "github.com/sourcegraph/sourcegraph/internal/gitserver/gitdomain" gitprotocol "github.com/sourcegraph/sourcegraph/internal/gitserver/protocol" + "github.com/sourcegraph/sourcegraph/internal/httpcli" "github.com/sourcegraph/sourcegraph/internal/observation" "github.com/sourcegraph/sourcegraph/internal/repos" diff --git a/internal/batches/service/service.go b/internal/batches/service/service.go index 6e07438af09..a31218e987c 100644 --- a/internal/batches/service/service.go +++ b/internal/batches/service/service.go @@ -1125,7 +1125,7 @@ func (s *Service) EnqueueChangesetSync(ctx context.Context, id int64) (err error ctx, _, endObservation := s.operations.enqueueChangesetSync.With(ctx, &err, observation.Args{}) defer endObservation(1, observation.Args{}) - // Check for existence of changeset so we don't swallow that error. + // Check for existence of changeset, so we don't swallow that error. changeset, err := s.store.GetChangeset(ctx, store.GetChangesetOpts{ID: id}) if err != nil { return err @@ -1222,10 +1222,10 @@ func (s *Service) ReenqueueChangeset(ctx context.Context, id int64) (changeset * // CheckNamespaceAccess checks whether the current user in the ctx has access // to either the user ID or the org ID as a namespace. -// If the userID is non-zero that will be checked. Otherwise the org ID will be +// If the userID is non-zero that will be checked. Otherwise, the org ID will be // checked. // If the current user is an admin, true will be returned. -// Otherwise it checks whether the current user _is_ the namespace user or has +// Otherwise, it checks whether the current user _is_ the namespace user or has // access to the namespace org. // If both values are zero, an error is returned. func (s *Service) CheckNamespaceAccess(ctx context.Context, namespaceUserID, namespaceOrgID int32) (err error) { @@ -1248,7 +1248,7 @@ var ErrNoNamespace = errors.New("no namespace given") // FetchUsernameForBitbucketServerToken fetches the username associated with a // Bitbucket server token. // -// We need the username in order to use the token as the password in a HTTP +// We need the username in order to use the token as the password in an HTTP // BasicAuth username/password pair used by gitserver to push commits. // // In order to not require from users to type in their BitbucketServer username @@ -1262,10 +1262,11 @@ func (s *Service) FetchUsernameForBitbucketServerToken(ctx context.Context, exte defer endObservation(1, observation.Args{}) // Get a changeset source for the external service and use the given authenticator. - css, err := s.sourcer.ForExternalService(ctx, s.store, &extsvcauth.OAuthBearerToken{Token: token}, sources.ForExternalServiceOpts{ - ExternalServiceType: externalServiceType, - ExternalServiceID: externalServiceID, - }, sources.AuthenticationStrategyUserCredential) + css, err := s.sourcer.ForExternalService(ctx, s.store, &extsvcauth.OAuthBearerToken{Token: token}, sources.SourcerOpts{ + ExternalServiceType: externalServiceType, + AuthenticationStrategy: sources.AuthenticationStrategyUserCredential, + ExternalServiceID: externalServiceID, + }) if err != nil { return "", err } @@ -1308,12 +1309,17 @@ func (s *Service) ValidateAuthenticator(ctx context.Context, a extsvcauth.Authen } // Get a changeset source for the external service and use the given authenticator. - css, err := s.sourcer.ForExternalService(ctx, s.store, a, sources.ForExternalServiceOpts{ - ExternalServiceType: args.ExternalServiceType, - ExternalServiceID: args.ExternalServiceID, - GitHubAppAccount: pointers.Deref(args.Username, ""), - GitHubAppKind: args.GitHubAppKind, - }, as) + css, err := s.sourcer.ForExternalService(ctx, s.store, a, sources.SourcerOpts{ + ExternalServiceType: args.ExternalServiceType, + ExternalServiceID: args.ExternalServiceID, + GitHubAppAccount: pointers.Deref(args.Username, ""), + GitHubAppKind: args.GitHubAppKind, + AuthenticationStrategy: as, + + // This is set to true because we want to validate the authenticator, not + // actually use it to create changesets. + AsNonCredential: true, + }) if err != nil { return err } @@ -1960,7 +1966,7 @@ func (s *Service) generateAuthenticatorForCredential(ctx context.Context, args g } a = auther } else if args.externalServiceType == extsvc.TypeBitbucketServer { - // We need to fetch the username for the token, as just an OAuth token isn't enough for some reason.. + // We need to fetch the username for the token, as just an OAuth token isn't enough for some reason. username, err := s.FetchUsernameForBitbucketServerToken(ctx, args.externalServiceURL, args.externalServiceType, args.credential) if err != nil { if bitbucketserver.IsUnauthorized(err) { diff --git a/internal/batches/sources/sources.go b/internal/batches/sources/sources.go index addbc78852f..f4a1833808f 100644 --- a/internal/batches/sources/sources.go +++ b/internal/batches/sources/sources.go @@ -3,13 +3,11 @@ package sources import ( "context" "fmt" - ghtypes "github.com/sourcegraph/sourcegraph/internal/github_apps/types" "net/url" "sort" "strings" "github.com/sourcegraph/log" - ghauth "github.com/sourcegraph/sourcegraph/internal/github_apps/auth" "github.com/sourcegraph/sourcegraph/internal/batches/store" btypes "github.com/sourcegraph/sourcegraph/internal/batches/types" @@ -19,8 +17,9 @@ import ( "github.com/sourcegraph/sourcegraph/internal/extsvc" "github.com/sourcegraph/sourcegraph/internal/extsvc/auth" "github.com/sourcegraph/sourcegraph/internal/extsvc/github" - ghaauth "github.com/sourcegraph/sourcegraph/internal/github_apps/auth" + ghauth "github.com/sourcegraph/sourcegraph/internal/github_apps/auth" ghastore "github.com/sourcegraph/sourcegraph/internal/github_apps/store" + ghtypes "github.com/sourcegraph/sourcegraph/internal/github_apps/types" "github.com/sourcegraph/sourcegraph/internal/gitserver" "github.com/sourcegraph/sourcegraph/internal/gitserver/protocol" "github.com/sourcegraph/sourcegraph/internal/httpcli" @@ -69,12 +68,11 @@ var ErrNoSSHCredential = errors.New("authenticator doesn't support SSH") type AuthenticationStrategy string const ( - // Authenticate using a traditional PAT configured by the user or site admin. This - // should be used for all code host interactions unless another authentication + // AuthenticationStrategyUserCredential is used to authenticate using a traditional PAT configured by + //the user or site admin. This should be used for all code host interactions unless another authentication // strategy is explicitly required. AuthenticationStrategyUserCredential AuthenticationStrategy = "USER_CREDENTIAL" - // Authenticate using a GitHub App. This should only be used for GitHub code hosts for - // commit signing interactions. + // AuthenticationStrategyGitHubApp is used to authenticate using a GitHub App. AuthenticationStrategyGitHubApp AuthenticationStrategy = "GITHUB_APP" ) @@ -90,6 +88,19 @@ type SourcerStore interface { GetChangesetSpecByID(ctx context.Context, id int64) (*btypes.ChangesetSpec, error) } +type SourcerOpts struct { + ExternalServiceID string + ExternalServiceType string + GitHubAppAccount string + GitHubAppKind ghtypes.GitHubAppKind + AuthenticationStrategy AuthenticationStrategy + + // We sometimes create a sourcer for a changeset that without the use of a credential. + // An example is when we want to create a GitHubSource for a changeset that is to + // be signed by a GitHub app. + AsNonCredential bool +} + // Sourcer exposes methods to get a ChangesetSource based on a changeset, repo or // external service. type Sourcer interface { @@ -108,13 +119,13 @@ type Sourcer interface { // // If the changeset was not created by a batch change, then a site credential will be // used. If another AuthenticationStrategy is specified, then it will be used. - ForChangeset(ctx context.Context, tx SourcerStore, ch *btypes.Changeset, as AuthenticationStrategy, repo *types.Repo, gitHubAppKind ghtypes.GitHubAppKind) (ChangesetSource, error) + ForChangeset(ctx context.Context, tx SourcerStore, ch *btypes.Changeset, repo *types.Repo, opts SourcerOpts) (ChangesetSource, error) // ForUser returns a ChangesetSource for changesets on the given repo. // It will be authenticated with the given authenticator. ForUser(ctx context.Context, tx SourcerStore, uid int32, repo *types.Repo) (ChangesetSource, error) // ForExternalService returns a ChangesetSource based on the provided external service opts. // It will be authenticated with the given authenticator. - ForExternalService(ctx context.Context, tx SourcerStore, au auth.Authenticator, opts ForExternalServiceOpts, as AuthenticationStrategy) (ChangesetSource, error) + ForExternalService(ctx context.Context, tx SourcerStore, au auth.Authenticator, opts SourcerOpts) (ChangesetSource, error) } // NewSourcer returns a new Sourcer to be used in Batch Changes. @@ -138,7 +149,7 @@ func newSourcer(cf *httpcli.Factory, csf changesetSourceFactory) Sourcer { } } -func (s *sourcer) ForChangeset(ctx context.Context, tx SourcerStore, ch *btypes.Changeset, as AuthenticationStrategy, targetRepo *types.Repo, gitHubAppKind ghtypes.GitHubAppKind) (ChangesetSource, error) { +func (s *sourcer) ForChangeset(ctx context.Context, tx SourcerStore, ch *btypes.Changeset, targetRepo *types.Repo, opts SourcerOpts) (ChangesetSource, error) { repo, err := tx.Repos().Get(ctx, ch.RepoID) if err != nil { return nil, errors.Wrap(err, "loading changeset repo") @@ -152,7 +163,7 @@ func (s *sourcer) ForChangeset(ctx context.Context, tx SourcerStore, ch *btypes. return nil, errors.Wrap(err, "loading external service") } - if as == AuthenticationStrategyGitHubApp && extSvc.Kind != extsvc.KindGitHub { + if opts.AuthenticationStrategy == AuthenticationStrategyGitHubApp && extSvc.Kind != extsvc.KindGitHub { return nil, ErrExternalServiceNotGitHub } @@ -161,48 +172,8 @@ func (s *sourcer) ForChangeset(ctx context.Context, tx SourcerStore, ch *btypes. return nil, err } - if as == AuthenticationStrategyGitHubApp { - cs, err := tx.GetChangesetSpecByID(ctx, ch.CurrentSpecID) - if err != nil { - return nil, errors.Wrap(err, "getting changeset spec") - } - - var owner string - // We check if the changeset is meant to be pushed to a fork. - // If yes, then we try to figure out the user namespace and get a github app for the user namespace. - if cs.IsFork() { - // forkNamespace is nil returns a non-nil value if the fork namespace is explicitly defined - // e.g sourcegraph. - // if it isn't then we assume the changeset will be forked into the current user's namespace - forkNamespace := cs.GetForkNamespace() - if forkNamespace != nil { - owner = *forkNamespace - } else { - u, err := getCloneURL(targetRepo) - if err != nil { - return nil, errors.Wrap(err, "getting url for forked changeset") - } - - owner, _, err = github.SplitRepositoryNameWithOwner(strings.TrimPrefix(u.Path, "/")) - if err != nil { - return nil, errors.Wrap(err, "getting owner from repo name") - } - } - } else { - // Get owner from repo metadata. We expect repo.Metadata to be a github.Repository because the - // authentication strategy `AuthenticationStrategyGitHubApp` only applies to GitHub repositories. - // so this is a safe type cast. - repoMetadata := repo.Metadata.(*github.Repository) - owner, _, err = github.SplitRepositoryNameWithOwner(repoMetadata.NameWithOwner) - if err != nil { - return nil, errors.Wrap(err, "getting owner from repo name") - } - } - - if gitHubAppKind == "" { - return nil, errors.New("ForChangeset with AuthenticationStrategyGitHubApp must be called with a gitHubAppKind, but was called with empty string.") - } - return withGitHubAppAuthenticator(ctx, tx, css, extSvc, owner, gitHubAppKind) + if opts.AsNonCredential { + return s.createChangesetSourceForGHApp(ctx, tx, css, extSvc, ch, targetRepo, repo, opts) } if ch.OwnedByBatchChangeID != 0 { @@ -217,6 +188,79 @@ func (s *sourcer) ForChangeset(ctx context.Context, tx SourcerStore, ch *btypes. return withSiteAuthenticator(ctx, tx, css, repo) } +// createChangesetSourceForGHApp creates a changeset source that is authenticated with a GitHub app. +func (s *sourcer) createChangesetSourceForGHApp( + ctx context.Context, + tx SourcerStore, + css ChangesetSource, + extSvc *types.ExternalService, + ch *btypes.Changeset, + targetRepo *types.Repo, + repo *types.Repo, + opts SourcerOpts, +) (ChangesetSource, error) { + if opts.AuthenticationStrategy != AuthenticationStrategyGitHubApp { + return nil, errors.New("commit signing is only supported for GitHub apps") + } + + var owner string + if ch != nil { + changesetOwner, err := getOwnerFromChangeset(ctx, tx, ch, targetRepo, repo) + if err != nil { + return nil, err + } + owner = changesetOwner + } else { + owner = opts.GitHubAppAccount + } + + if opts.GitHubAppKind == "" { + return nil, errors.New("ForChangeset with AuthenticationStrategyGitHubApp must be called with a gitHubAppKind, but was called with empty string.") + } + return withGitHubAppAuthenticator(ctx, tx, css, extSvc, owner, opts.GitHubAppKind) +} + +func getOwnerFromChangeset(ctx context.Context, tx SourcerStore, ch *btypes.Changeset, targetRepo, repo *types.Repo) (string, error) { + cs, err := tx.GetChangesetSpecByID(ctx, ch.CurrentSpecID) + if err != nil { + return "", errors.Wrap(err, "getting changeset spec") + } + + var owner string + // We check if the changeset is meant to be pushed to a fork. + // If yes, then we try to figure out the user namespace and get a GitHub app for the user namespace. + if cs.IsFork() { + // forkNamespace is nil returns a non-nil value if the fork namespace is explicitly defined + // e.g. Sourcegraph. + // if it isn't then we assume the changeset will be forked into the current user's namespace + forkNamespace := cs.GetForkNamespace() + if forkNamespace != nil { + owner = *forkNamespace + } else { + u, err := getCloneURL(targetRepo) + if err != nil { + return "", errors.Wrap(err, "getting url for forked changeset") + } + + owner, _, err = github.SplitRepositoryNameWithOwner(strings.TrimPrefix(u.Path, "/")) + if err != nil { + return "", errors.Wrap(err, "getting owner from repo name") + } + } + } else { + // Get owner from repo metadata. We expect repo.Metadata to be a github.Repository because the + // authentication strategy `AuthenticationStrategyGitHubApp` only applies to GitHub repositories. + // so this is a safe type cast. + repoMetadata := repo.Metadata.(*github.Repository) + owner, _, err = github.SplitRepositoryNameWithOwner(repoMetadata.NameWithOwner) + if err != nil { + return "", errors.Wrap(err, "getting owner from repo name") + } + } + + return owner, nil +} + func (s *sourcer) ForUser(ctx context.Context, tx SourcerStore, uid int32, repo *types.Repo) (ChangesetSource, error) { // Consider all available external services for this repo. extSvc, err := loadExternalService(ctx, tx.ExternalServices(), database.ExternalServicesListOptions{ @@ -232,14 +276,7 @@ func (s *sourcer) ForUser(ctx context.Context, tx SourcerStore, uid int32, repo return withAuthenticatorForUser(ctx, tx, css, uid, repo) } -type ForExternalServiceOpts struct { - ExternalServiceID string - ExternalServiceType string - GitHubAppAccount string - GitHubAppKind ghtypes.GitHubAppKind -} - -func (s *sourcer) ForExternalService(ctx context.Context, tx SourcerStore, au auth.Authenticator, opts ForExternalServiceOpts, as AuthenticationStrategy) (ChangesetSource, error) { +func (s *sourcer) ForExternalService(ctx context.Context, tx SourcerStore, au auth.Authenticator, opts SourcerOpts) (ChangesetSource, error) { // Empty authenticators are not allowed. if au == nil { return nil, ErrMissingCredentials @@ -264,8 +301,9 @@ func (s *sourcer) ForExternalService(ctx context.Context, tx SourcerStore, au au if err != nil { return nil, err } - if as == AuthenticationStrategyGitHubApp { - return withGitHubAppAuthenticator(ctx, tx, css, extSvc, opts.GitHubAppAccount, opts.GitHubAppKind) + + if opts.AsNonCredential { + return s.createChangesetSourceForGHApp(ctx, tx, css, extSvc, nil, nil, nil, opts) } return css.WithAuthenticator(au) } @@ -399,7 +437,7 @@ func withGitHubAppAuthenticator(ctx context.Context, tx SourcerStore, css Change return nil, ErrNoGitHubAppInstallation } - appAuther, err := ghaauth.NewGitHubAppAuthenticator(app.AppID, []byte(app.PrivateKey)) + appAuther, err := ghauth.NewGitHubAppAuthenticator(app.AppID, []byte(app.PrivateKey)) if err != nil { return nil, errors.Wrap(err, "creating GitHub App authenticator") } @@ -416,7 +454,7 @@ func withGitHubAppAuthenticator(ctx context.Context, tx SourcerStore, css Change // a GitHub App authenticated on behalf of a user, we should switch to using that // access token here. See here for more details: // https://docs.github.com/en/apps/creating-github-apps/authenticating-with-a-github-app/about-authentication-with-a-github-app - installationAuther := ghaauth.NewInstallationAccessToken(baseURL, installID, appAuther, keyring.Default().GitHubAppKey) + installationAuther := ghauth.NewInstallationAccessToken(baseURL, installID, appAuther, keyring.Default().GitHubAppKey) return css.WithAuthenticator(installationAuther) } diff --git a/internal/batches/sources/sources_test.go b/internal/batches/sources/sources_test.go index 1103eef9c2f..3864f4ecfc5 100644 --- a/internal/batches/sources/sources_test.go +++ b/internal/batches/sources/sources_test.go @@ -411,7 +411,9 @@ func TestSourcer_ForChangeset(t *testing.T) { return want, nil }) - have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyUserCredential, repo, "") + have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, repo, SourcerOpts{ + AuthenticationStrategy: AuthenticationStrategyUserCredential, + }) assert.NoError(t, err) assert.Same(t, want, have) }) @@ -452,7 +454,7 @@ func TestSourcer_ForChangeset(t *testing.T) { return want, nil }) - have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyUserCredential, repo, "") + have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, repo, SourcerOpts{AuthenticationStrategy: AuthenticationStrategyUserCredential}) assert.NoError(t, err) assert.Same(t, want, have) }) @@ -487,7 +489,7 @@ func TestSourcer_ForChangeset(t *testing.T) { tx.ExternalServicesFunc.SetDefaultReturn(extsvcStore) css := NewMockChangesetSource() - _, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyUserCredential, repo, "") + _, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, repo, SourcerOpts{AuthenticationStrategy: AuthenticationStrategyUserCredential}) assert.Error(t, err) }) @@ -537,7 +539,11 @@ func TestSourcer_ForChangeset(t *testing.T) { return want, nil }) - have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyGitHubApp, repo, ghatypes.SiteCredentialGitHubAppKind) + have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, repo, SourcerOpts{ + AuthenticationStrategy: AuthenticationStrategyGitHubApp, + GitHubAppKind: ghatypes.SiteCredentialGitHubAppKind, + AsNonCredential: true, + }) assert.NoError(t, err) assert.Same(t, want, have) }) @@ -600,7 +606,11 @@ func TestSourcer_ForChangeset(t *testing.T) { }, } - have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyGitHubApp, targetRepo, ghatypes.SiteCredentialGitHubAppKind) + have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, targetRepo, SourcerOpts{ + AuthenticationStrategy: AuthenticationStrategyGitHubApp, + GitHubAppKind: ghatypes.SiteCredentialGitHubAppKind, + AsNonCredential: true, + }) assert.NoError(t, err) assert.Same(t, want, have) }) @@ -633,7 +643,7 @@ func TestSourcer_ForChangeset(t *testing.T) { return want, nil }) - have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyUserCredential, repo, "") + have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, repo, SourcerOpts{AuthenticationStrategy: AuthenticationStrategyUserCredential}) assert.NoError(t, err) assert.Same(t, want, have) }) @@ -659,7 +669,7 @@ func TestSourcer_ForChangeset(t *testing.T) { want := errors.New("validator was called") css.ValidateAuthenticatorFunc.SetDefaultReturn(want) - _, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyUserCredential, repo, "") + _, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, repo, SourcerOpts{AuthenticationStrategy: AuthenticationStrategyUserCredential}) assert.Error(t, err) }) }) diff --git a/internal/batches/sources/testing/BUILD.bazel b/internal/batches/sources/testing/BUILD.bazel index f2c53f328b4..2ed1d705a6b 100644 --- a/internal/batches/sources/testing/BUILD.bazel +++ b/internal/batches/sources/testing/BUILD.bazel @@ -10,7 +10,6 @@ go_library( "//internal/batches/sources", "//internal/batches/types", "//internal/extsvc/auth", - "//internal/github_apps/types", "//internal/gitserver/protocol", "//internal/types", "//lib/errors", diff --git a/internal/batches/sources/testing/fake.go b/internal/batches/sources/testing/fake.go index b75a39910d3..00c666be132 100644 --- a/internal/batches/sources/testing/fake.go +++ b/internal/batches/sources/testing/fake.go @@ -2,7 +2,6 @@ package testing import ( "context" - ghtypes "github.com/sourcegraph/sourcegraph/internal/github_apps/types" "github.com/sourcegraph/sourcegraph/internal/batches/sources" btypes "github.com/sourcegraph/sourcegraph/internal/batches/types" @@ -25,7 +24,7 @@ type fakeSourcer struct { source sources.ChangesetSource } -func (s *fakeSourcer) ForChangeset(ctx context.Context, tx sources.SourcerStore, ch *btypes.Changeset, as sources.AuthenticationStrategy, repo *types.Repo, gitHubAppKind ghtypes.GitHubAppKind) (sources.ChangesetSource, error) { +func (s *fakeSourcer) ForChangeset(_ context.Context, _ sources.SourcerStore, _ *btypes.Changeset, _ *types.Repo, _ sources.SourcerOpts) (sources.ChangesetSource, error) { return s.source, s.err } @@ -33,7 +32,7 @@ func (s *fakeSourcer) ForUser(ctx context.Context, tx sources.SourcerStore, uid return s.source, s.err } -func (s *fakeSourcer) ForExternalService(ctx context.Context, tx sources.SourcerStore, au auth.Authenticator, opts sources.ForExternalServiceOpts, as sources.AuthenticationStrategy) (sources.ChangesetSource, error) { +func (s *fakeSourcer) ForExternalService(_ context.Context, _ sources.SourcerStore, _ auth.Authenticator, _ sources.SourcerOpts) (sources.ChangesetSource, error) { return s.source, s.err } diff --git a/internal/batches/syncer/syncer.go b/internal/batches/syncer/syncer.go index a62d6459bd4..8e547947416 100644 --- a/internal/batches/syncer/syncer.go +++ b/internal/batches/syncer/syncer.go @@ -497,7 +497,9 @@ func (s *changesetSyncer) SyncChangeset(ctx context.Context, id int64) error { } srcer := sources.NewSourcer(s.httpFactory) - source, err := srcer.ForChangeset(ctx, s.syncStore, cs, sources.AuthenticationStrategyUserCredential, repo, "") + source, err := srcer.ForChangeset(ctx, s.syncStore, cs, repo, sources.SourcerOpts{ + AuthenticationStrategy: sources.AuthenticationStrategyUserCredential, + }) if err != nil { if errors.Is(err, store.ErrDeletedNamespace) { syncLogger.Debug("SyncChangeset skipping changeset: namespace deleted")