diff --git a/appconfig/appconfig.go b/appconfig/appconfig.go index 16e2b6f0..00adf55b 100644 --- a/appconfig/appconfig.go +++ b/appconfig/appconfig.go @@ -27,6 +27,7 @@ import ( "strings" "github.com/google/go-github/v90/github" + "github.com/palantir/go-githubapp/githubapp" "github.com/rs/zerolog" ) @@ -81,6 +82,9 @@ type Loader struct { parser RemoteRefParser defaultRepo string defaultPaths []string + + clientCreator githubapp.ClientCreator + installations githubapp.InstallationsService } // NewLoader creates a Loader that loads configuration from paths. @@ -139,7 +143,7 @@ func (ld *Loader) LoadConfig(ctx context.Context, client *github.Client, owner, } if remote != nil { logger.Debug().Msgf("Found remote configuration at %s in %s", p, c.Source) - return ld.loadRemoteConfig(ctx, client, *remote, c) + return ld.loadRemoteConfig(ctx, client, owner, *remote, c) } } @@ -159,7 +163,7 @@ func (ld *Loader) LoadConfig(ctx context.Context, client *github.Client, owner, return Config{}, nil } -func (ld *Loader) loadRemoteConfig(ctx context.Context, client *github.Client, remote RemoteRef, c Config) (Config, error) { +func (ld *Loader) loadRemoteConfig(ctx context.Context, client *github.Client, sourceOwner string, remote RemoteRef, c Config) (Config, error) { logger := zerolog.Ctx(ctx) notFoundErr := fmt.Errorf("invalid remote reference: file does not exist") @@ -167,6 +171,7 @@ func (ld *Loader) loadRemoteConfig(ctx context.Context, client *github.Client, r if err != nil { return c, err } + client = ld.remoteClient(ctx, client, sourceOwner, owner, repo) path := remote.Path if path == "" && len(ld.paths) > 0 { @@ -250,7 +255,7 @@ func (ld *Loader) loadDefaultConfig(ctx context.Context, client *github.Client, } if remote != nil { logger.Debug().Msgf("Found remote default configuration at %s in %s", p, c.Source) - return ld.loadRemoteConfig(ctx, client, *remote, c) + return ld.loadRemoteConfig(ctx, client, owner, *remote, c) } } @@ -264,6 +269,30 @@ func (ld *Loader) loadDefaultConfig(ctx context.Context, client *github.Client, return Config{}, nil } +// remoteClient returns an installation-scoped client for remoteOwner/remoteRepo when +// private remotes are enabled and the remote belongs to a different owner. If +// the app is not installed on that repository, or a client cannot be created, it +// falls back to the caller's client so public repositories continue to work. +func (ld *Loader) remoteClient(ctx context.Context, client *github.Client, sourceOwner, remoteOwner, remoteRepo string) *github.Client { + if strings.EqualFold(sourceOwner, remoteOwner) || ld.clientCreator == nil || ld.installations == nil { + return client + } + + logger := zerolog.Ctx(ctx) + installation, err := ld.installations.GetByRepository(ctx, remoteOwner, remoteRepo) + if err != nil { + logger.Warn().Err(err).Msgf("Failed to find GitHub App installation for remote configuration repository %q; using the original client", remoteOwner+"/"+remoteRepo) + return client + } + + remoteClient, err := ld.clientCreator.NewInstallationClient(installation.ID) + if err != nil { + logger.Warn().Err(err).Msgf("Failed to create GitHub App client for remote configuration repository %q; using the original client", remoteOwner+"/"+remoteRepo) + return client + } + return remoteClient +} + // getFileContents returns the content of the file at path on ref in owner/repo // if it exists. Returns an empty slice and false if the file does not exist. func getFileContents(ctx context.Context, client *github.Client, owner, repo, ref, path string) ([]byte, bool, error) { diff --git a/appconfig/appconfig_test.go b/appconfig/appconfig_test.go index 9463c477..3d7f6bac 100644 --- a/appconfig/appconfig_test.go +++ b/appconfig/appconfig_test.go @@ -17,11 +17,13 @@ package appconfig import ( "bytes" "context" + "errors" "net/http" "path/filepath" "testing" "github.com/google/go-github/v90/github" + "github.com/palantir/go-githubapp/githubapp" ) const ( @@ -29,6 +31,41 @@ const ( TestRef = "develop" ) +type testInstallationsService struct { + githubapp.InstallationsService + + installation githubapp.Installation + err error + calls int + owner string + repo string +} + +func (s *testInstallationsService) GetByRepository(_ context.Context, owner, repo string) (githubapp.Installation, error) { + s.calls++ + s.owner = owner + s.repo = repo + if s.err != nil { + return githubapp.Installation{}, s.err + } + return s.installation, nil +} + +type testClientCreator struct { + githubapp.ClientCreator + + client *github.Client + err error + calls int + installationID int64 +} + +func (c *testClientCreator) NewInstallationClient(installationID int64) (*github.Client, error) { + c.calls++ + c.installationID = installationID + return c.client, c.err +} + func TestLoadConfig(t *testing.T) { tests := map[string]struct { Paths []string @@ -142,6 +179,91 @@ func TestLoadConfig(t *testing.T) { } } +func TestLoadConfigWithPrivateRemotes(t *testing.T) { + newClient := func(rp *ResponsePlayer) *github.Client { + client, _ := github.NewClient(github.WithHTTPClient(&http.Client{Transport: rp})) + return client + } + localRules := func(includeRemote bool) *ResponsePlayer { + rp := &ResponsePlayer{} + rp.AddRule(ExactPathMatcher("/repos/test/remote-ref/contents/.github/test-app.yml"), filepath.Join("testdata", "remote-ref-contents.yml")) + if includeRemote { + rp.AddRule(ExactPathMatcher("/repos/remote/config/contents/config/test-app.yml"), filepath.Join("testdata", "config-contents.yml")) + } + return rp + } + + t.Run("uses remote repository installation client", func(t *testing.T) { + localRules := localRules(false) + remoteRules := &ResponsePlayer{} + remoteRule := remoteRules.AddRule(ExactPathMatcher("/repos/remote/config/contents/config/test-app.yml"), filepath.Join("testdata", "config-contents.yml")) + installations := &testInstallationsService{installation: githubapp.Installation{ID: 42, Owner: "remote"}} + creator := &testClientCreator{client: newClient(remoteRules)} + + loader := NewLoader([]string{".github/test-app.yml"}, WithPrivateRemotes(creator, installations)) + cfg, err := loader.LoadConfig(context.Background(), newClient(localRules), TestOwner, "remote-ref", TestRef) + if err != nil { + t.Fatalf("unexpected error loading config: %v", err) + } + if !bytes.Equal(cfg.Content, []byte("message: hello\n")) { + t.Errorf("incorrect content: %s", cfg.Content) + } + if installations.calls != 1 || creator.calls != 1 || remoteRule.Count != 1 { + t.Errorf("expected one installation lookup, client creation, and remote request; got %d, %d, and %d", installations.calls, creator.calls, remoteRule.Count) + } + if installations.owner != "remote" || installations.repo != "config" { + t.Errorf("expected installation lookup for remote/config, got %s/%s", installations.owner, installations.repo) + } + if creator.installationID != 42 { + t.Errorf("expected installation client for ID 42, got %d", creator.installationID) + } + }) + + for name, test := range map[string]struct { + installationsErr error + creatorErr error + }{ + "falls back when remote repository has no installation": {installationsErr: githubapp.InstallationNotFound("remote/config")}, + "falls back when installation client creation fails": {creatorErr: errors.New("client creation failed")}, + } { + t.Run(name, func(t *testing.T) { + localRules := localRules(true) + installations := &testInstallationsService{installation: githubapp.Installation{ID: 42, Owner: "remote"}, err: test.installationsErr} + creator := &testClientCreator{err: test.creatorErr} + + loader := NewLoader([]string{".github/test-app.yml"}, WithPrivateRemotes(creator, installations)) + cfg, err := loader.LoadConfig(context.Background(), newClient(localRules), TestOwner, "remote-ref", TestRef) + if err != nil { + t.Fatalf("unexpected error loading config: %v", err) + } + if !bytes.Equal(cfg.Content, []byte("message: hello\n")) { + t.Errorf("incorrect content: %s", cfg.Content) + } + if test.installationsErr != nil && creator.calls != 0 { + t.Errorf("client creator was called after installation lookup failed") + } + if test.creatorErr != nil && creator.calls != 1 { + t.Errorf("expected one client creation attempt, got %d", creator.calls) + } + }) + } +} + +func TestPrivateRemotesSameOwnerReusesOriginalClient(t *testing.T) { + client := makeTestClient() + installations := &testInstallationsService{} + creator := &testClientCreator{} + loader := NewLoader(nil, WithPrivateRemotes(creator, installations)) + + got := loader.remoteClient(context.Background(), client, "Source-Owner", "source-owner", "config") + if got != client { + t.Error("same-owner remote did not reuse the original client") + } + if installations.calls != 0 || creator.calls != 0 { + t.Errorf("same-owner remote attempted installation lookup or client creation: %d, %d", installations.calls, creator.calls) + } +} + func makeTestClient() *github.Client { rp := &ResponsePlayer{} for route, f := range map[string]string{ diff --git a/appconfig/options.go b/appconfig/options.go index 4f5cd83f..7285d443 100644 --- a/appconfig/options.go +++ b/appconfig/options.go @@ -14,6 +14,10 @@ package appconfig +import ( + "github.com/palantir/go-githubapp/githubapp" +) + type Option func(*Loader) // WithRemoteRefParser sets the parser for encoded RemoteRefs. The default @@ -36,20 +40,25 @@ func WithOwnerDefault(name string, paths []string) Option { } } -/* - -Not sure this is valuable yet, but leaving this option function as a starting -point for a future implementation. See https://github.com/palantir/policy-bot/issues/111 -for some explanation of why this is desired. - -In the Loader implementation, if a ClientCreator and InstallationsService are -set, the loadRemoteConfig method would use them to create a new client if the -remote owner does not equal the starting owner. - // WithPrivateRemotes enables loading remote configuration from private -// repositories in different organizations. By default, only public -// repositories can be remote targets. -func WithPrivateRemotes(cc githubapp.ClientCreator, installs githubapp.InstallationsService) Option { - return func(ld *Loader) {} +// repositories owned by a different user or organization. It uses the app's +// installation on the remote repository to fetch the referenced file. If the +// app is not installed on that repository, or an installation client cannot be created, +// the loader logs the failure and falls back to the original client so public +// remote repositories remain supported. +// +// WARNING: Enabling this option can expose configuration file content and +// repository existence to users who otherwise may not be able to access them. +// Enable it only when the app is installed on GitHub organizations where all +// users are trusted, or when the caller otherwise prevents unintentional +// information disclosure. +// +// This loader does not cache installation lookups or clients. Callers that +// load configuration frequently should pass caching implementations, such as +// githubapp.NewCachingInstallationsService and githubapp.NewCachingClientCreator. +func WithPrivateRemotes(clientCreator githubapp.ClientCreator, installations githubapp.InstallationsService) Option { + return func(ld *Loader) { + ld.clientCreator = clientCreator + ld.installations = installations + } } -*/