Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Commit 8eb5d83

Browse files
batches: fix commit signing with changeset forks (#57520)
[Context](https://sourcegraph.slack.com/archives/C04UV9ZVBB2/p1696374182439959) The customer reported an issue where a Sourcegraph instance configured with a GitHub app for Batch Changes couldn't sign commits on forked changesets even when the GitHub app was granted access to the forked repo. This happens because while creating an authenticator used to create the signed commits, we always assume the repository to find the GitHub app installation for is the original repository where the Batch CHange was executed. https://github.com/sourcegraph/sourcegraph/blob/main/internal/batches/sources/sources.go#L161 In this PR, I changed the approach and instead checked if the changeset is pushed to a fork, then figured out the namespace where the forked changeset is created and used that to get the GitHub app installation record. ![CleanShot 2023-10-11 at 11 49 50@2x](https://github.com/sourcegraph/sourcegraph/assets/25608335/3f90937e-6d1f-4631-8ce4-ca7a06a8423f) ## Test plan * MAnual testing - Add a GitHub app for BAtch Changes commit signing. Ensure you give it access to the forked repository of your choice. - Add `batchChanges.enforceForks` to your site config, and create a batch change. - The changeset should be verified and created in your repository fork once published. * Updated unit tests
1 parent e100408 commit 8eb5d83

File tree

12 files changed

+375
-21
lines changed

12 files changed

+375
-21
lines changed

cmd/frontend/internal/batches/resolvers/changeset_counts_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ func TestChangesetCountsOverTimeIntegration(t *testing.T) {
190190
t.Fatal(err)
191191
}
192192

193-
src, err := sourcer.ForChangeset(ctx, bstore, c, sources.AuthenticationStrategyUserCredential)
193+
src, err := sourcer.ForChangeset(ctx, bstore, c, sources.AuthenticationStrategyUserCredential, githubRepo)
194194
if err != nil {
195195
t.Fatalf("failed to build source for repo: %s", err)
196196
}

cmd/frontend/internal/batches/webhooks/bitbucketserver_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ func testBitbucketServerWebhook(db database.DB, userID int32) func(*testing.T) {
167167
if err := s.CreateChangeset(ctx, ch); err != nil {
168168
t.Fatal(err)
169169
}
170-
src, err := sourcer.ForChangeset(ctx, s, ch, sources.AuthenticationStrategyUserCredential)
170+
src, err := sourcer.ForChangeset(ctx, s, ch, sources.AuthenticationStrategyUserCredential, bitbucketRepo)
171171
if err != nil {
172172
t.Fatal(err)
173173
}

cmd/frontend/internal/batches/webhooks/github_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ func testGitHubWebhook(db database.DB, userID int32) func(*testing.T) {
156156
})
157157
defer state.Unmock()
158158

159-
src, err := sourcer.ForChangeset(ctx, s, changeset, sources.AuthenticationStrategyUserCredential)
159+
src, err := sourcer.ForChangeset(ctx, s, changeset, sources.AuthenticationStrategyUserCredential, githubRepo)
160160
if err != nil {
161161
t.Fatal(err)
162162
}

internal/batches/reconciler/executor.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -592,7 +592,7 @@ func (e *executor) decorateChangesetBody(ctx context.Context) (string, error) {
592592
}
593593

594594
func loadChangesetSource(ctx context.Context, s *store.Store, sourcer sources.Sourcer, ch *btypes.Changeset, repo *types.Repo) (sources.ChangesetSource, error) {
595-
css, err := sourcer.ForChangeset(ctx, s, ch, sources.AuthenticationStrategyUserCredential)
595+
css, err := sourcer.ForChangeset(ctx, s, ch, sources.AuthenticationStrategyUserCredential, repo)
596596
if err != nil {
597597
switch err {
598598
case sources.ErrMissingCredentials:
@@ -648,7 +648,7 @@ func (e *executor) runAfterCommit(ctx context.Context, css sources.ChangesetSour
648648
// configured for Batch Changes to sign commits on this code host with.
649649
if _, ok := css.(*sources.GitHubSource); ok {
650650
// Attempt to get a ChangesetSource authenticated with a GitHub App.
651-
css, err = e.sourcer.ForChangeset(ctx, e.tx, e.ch, sources.AuthenticationStrategyGitHubApp)
651+
css, err = e.sourcer.ForChangeset(ctx, e.tx, e.ch, sources.AuthenticationStrategyGitHubApp, e.remote)
652652
if err != nil {
653653
switch err {
654654
case sources.ErrNoGitHubAppConfigured:

internal/batches/sources/mocks_test.go

Lines changed: 127 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

internal/batches/sources/sources.go

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@ type SourcerStore interface {
8585
ExternalServices() database.ExternalServiceStore
8686
UserCredentials() database.UserCredentialsStore
8787
GitHubAppsStore() ghastore.GitHubAppsStore
88+
GetChangesetSpecByID(ctx context.Context, id int64) (*btypes.ChangesetSpec, error)
8889
}
8990

9091
// Sourcer exposes methods to get a ChangesetSource based on a changeset, repo or
@@ -105,7 +106,7 @@ type Sourcer interface {
105106
//
106107
// If the changeset was not created by a batch change, then a site credential will be
107108
// used. If another AuthenticationStrategy is specified, then it will be used.
108-
ForChangeset(ctx context.Context, tx SourcerStore, ch *btypes.Changeset, as AuthenticationStrategy) (ChangesetSource, error)
109+
ForChangeset(ctx context.Context, tx SourcerStore, ch *btypes.Changeset, as AuthenticationStrategy, repo *types.Repo) (ChangesetSource, error)
109110
// ForUser returns a ChangesetSource for changesets on the given repo.
110111
// It will be authenticated with the given authenticator.
111112
ForUser(ctx context.Context, tx SourcerStore, uid int32, repo *types.Repo) (ChangesetSource, error)
@@ -135,7 +136,7 @@ func newSourcer(cf *httpcli.Factory, csf changesetSourceFactory) Sourcer {
135136
}
136137
}
137138

138-
func (s *sourcer) ForChangeset(ctx context.Context, tx SourcerStore, ch *btypes.Changeset, as AuthenticationStrategy) (ChangesetSource, error) {
139+
func (s *sourcer) ForChangeset(ctx context.Context, tx SourcerStore, ch *btypes.Changeset, as AuthenticationStrategy, targetRepo *types.Repo) (ChangesetSource, error) {
139140
repo, err := tx.Repos().Get(ctx, ch.RepoID)
140141
if err != nil {
141142
return nil, errors.Wrap(err, "loading changeset repo")
@@ -159,10 +160,41 @@ func (s *sourcer) ForChangeset(ctx context.Context, tx SourcerStore, ch *btypes.
159160
}
160161

161162
if as == AuthenticationStrategyGitHubApp {
162-
repoMetadata := repo.Metadata.(*github.Repository)
163-
owner, _, err := github.SplitRepositoryNameWithOwner(repoMetadata.NameWithOwner)
163+
cs, err := tx.GetChangesetSpecByID(ctx, ch.CurrentSpecID)
164164
if err != nil {
165-
return nil, errors.Wrap(err, "getting owner from repo name")
165+
return nil, errors.Wrap(err, "getting changeset spec")
166+
}
167+
168+
var owner string
169+
// We check if the changeset is meant to be pushed to a fork.
170+
// If yes, then we try to figure out the user namespace and get a github app for the user namespace.
171+
if cs.IsFork() {
172+
// forkNamespace is nil returns a non-nil value if the fork namespace is explicitly defined
173+
// e.g sourcegraph.
174+
// if it isn't then we assume the changeset will be forked into the current user's namespace
175+
forkNamespace := cs.GetForkNamespace()
176+
if forkNamespace != nil {
177+
owner = *forkNamespace
178+
} else {
179+
u, err := getCloneURL(targetRepo)
180+
if err != nil {
181+
return nil, errors.Wrap(err, "getting url for forked changeset")
182+
}
183+
184+
owner, _, err = github.SplitRepositoryNameWithOwner(strings.TrimPrefix(u.Path, "/"))
185+
if err != nil {
186+
return nil, errors.Wrap(err, "getting owner from repo name")
187+
}
188+
}
189+
} else {
190+
// Get owner from repo metadata. We expect repo.Metadata to be a github.Repository because the
191+
// authentication strategy `AuthenticationStrategyGitHubApp` only applies to GitHub repositories.
192+
// so this is a safe type cast.
193+
repoMetadata := repo.Metadata.(*github.Repository)
194+
owner, _, err = github.SplitRepositoryNameWithOwner(repoMetadata.NameWithOwner)
195+
if err != nil {
196+
return nil, errors.Wrap(err, "getting owner from repo name")
197+
}
166198
}
167199

168200
return withGitHubAppAuthenticator(ctx, tx, css, extSvc, owner)
@@ -542,7 +574,7 @@ func getCloneURL(repo *types.Repo) (*vcs.URL, error) {
542574
}
543575

544576
sort.SliceStable(parsedURLs, func(i, j int) bool {
545-
return !parsedURLs[i].IsSSH()
577+
return !parsedURLs[i].IsSSH() && parsedURLs[j].IsSSH()
546578
})
547579

548580
return parsedURLs[0], nil

0 commit comments

Comments
 (0)