Repository: cli/cli
File: pkg/cmd/factory/remote_resolver_test.go (Go)

Code before:
package factory

import (
	"errors"
	"net/url"
	"testing"

	"github.com/cli/cli/v2/git"
	"github.com/cli/cli/v2/internal/config"
	"github.com/cli/cli/v2/internal/gh"
	ghmock "github.com/cli/cli/v2/internal/gh/mock"
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
)

type identityTranslator struct{}

func (it identityTranslator) Translate(u *url.URL) *url.URL {
	return u
}

func Test_remoteResolver_Caching(t *testing.T) {
	tests := []struct {
		name     string
		remotes  func() (git.RemoteSet, error)
		config   gh.Config
		output   []string
		wantsErr bool
	}{
		{
			name: "caching error resolution",
			remotes: func() (git.RemoteSet, error) {
				return nil, errors.New("failed to read remotes")
			},
			config: func() gh.Config {
				cfg := &ghmock.ConfigMock{}
				cfg.AuthenticationFunc = func() gh.AuthConfig {
					authCfg := &config.AuthConfig{}
					authCfg.SetHosts([]string{"github.com"})
					authCfg.SetDefaultHost("github.com", "default")
					return authCfg
				}
				return cfg
			}(),
			output:   nil,
			wantsErr: true,
		},

Review comment (security):
I think the implementation is correct but I'm not convinced this is actually testing the caching behaviour. Removing the caching entirely from `Resolve` still passes these tests:

```go
func (rr *remoteResolver) Resolver() func() (context.Remotes, error) {
	return func() (context.Remotes, error) {
		// if rr.cachedRemotes != nil || rr.remotesError != nil {
		// 	return rr.cachedRemotes, rr.remotesError
		// }

		var remotesErr error

		gitRemotes, err := rr.readRemotes()
		if err != nil {
			remotesErr = err
			return nil, err
		}
		if len(gitRemotes) == 0 {
			remotesErr = errors.New("no git remotes found")
			return nil, remotesErr
		}

		sshTranslate := rr.urlTranslator
		if sshTranslate == nil {
			sshTranslate = ssh.NewTranslator()
		}
		resolvedRemotes := context.TranslateRemotes(gitRemotes, sshTranslate)

		cfg, err := rr.getConfig()
		if err != nil {
			return nil, err
		}

		authedHosts := cfg.Authentication().Hosts()
		if len(authedHosts) == 0 {
			return nil, errors.New("could not find any host configurations")
		}
		defaultHost, src := cfg.Authentication().DefaultHost()

		// Use set to dedupe list of hosts
		hostsSet := set.NewStringSet()
		hostsSet.AddValues(authedHosts)
		hostsSet.AddValues([]string{defaultHost, ghinstance.Default()})
		hosts := hostsSet.ToSlice()

		// Sort remotes
		sort.Sort(resolvedRemotes)

		cachedRemotes := resolvedRemotes.FilterByHosts(hosts)

		// Filter again by default host if one is set
		// For config file default host fallback to cachedRemotes if none match
		// For environment default host (GH_HOST) do not fallback to cachedRemotes if none match
		if src != "default" {
			filteredRemotes := cachedRemotes.FilterByHosts([]string{defaultHost})
			if isHostEnv(src) || len(filteredRemotes) > 0 {
				cachedRemotes = filteredRemotes
			}
		}

		if len(cachedRemotes) == 0 {
			if isHostEnv(src) {
				remotesErr = fmt.Errorf("none of the git remotes configured for this repository correspond to the %s environment variable. Try adding a matching remote or unsetting the variable", src)
				return nil, remotesErr
			} else if cfg.Authentication().HasEnvToken() {
				remotesErr = errors.New("set the GH_HOST environment variable to specify which GitHub host to use")
				return nil, remotesErr
			}
			remotesErr = errors.New("none of the git remotes configured for this repository point to a known GitHub host. To tell gh about a new GitHub host, please use `gh auth login`")
			return nil, remotesErr
		}

		return cachedRemotes, nil
	}
}
```

If you look at my original [test](https://github.com/cli/cli/issues/10103#issue-2750771557):

```go
func TestRemoteResolverCachesRemotes(t *testing.T) {
	var readRemotesCalled bool

	rr := &remoteResolver{
		readRemotes: func() (git.RemoteSet, error) {
			if readRemotesCalled {
				return git.RemoteSet{}, errors.New("readRemotes should only be called once")
			}

			readRemotesCalled = true
			return git.RemoteSet{
				git.NewRemote("origin", "https://github.com/owner/repo.git"),
			}, nil
		},
		getConfig: func() (gh.Config, error) {
			cfg := &ghmock.ConfigMock{}
			cfg.AuthenticationFunc = func() gh.AuthConfig {
				authCfg := &config.AuthConfig{}
				authCfg.SetHosts([]string{"github.com"})
				authCfg.SetDefaultHost("github.com", "default")
				return authCfg
			}
			return cfg, nil
		},
		urlTranslator: identityTranslator{},
	}

	resolver := rr.Resolver()
	_, err := resolver()
	require.NoError(t, err)
	_, err = resolver()
	require.NoError(t, err)
}
```

There is a a conditional inside `ReadRemotes` to alter the behaviour based on whether the function had previously been called, allowing for us to validate that this "expensive" function is only called once.

Code after:
package factory

import (
	"errors"
	"net/url"
	"testing"

	"github.com/cli/cli/v2/context"
	"github.com/cli/cli/v2/git"
	"github.com/cli/cli/v2/internal/config"
	"github.com/cli/cli/v2/internal/gh"
	ghmock "github.com/cli/cli/v2/internal/gh/mock"
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
)

type identityTranslator struct{}

func (it identityTranslator) Translate(u *url.URL) *url.URL {
	return u
}

func Test_remoteResolver(t *testing.T) {
	tests := []struct {
		name     string
		remotes  func() (git.RemoteSet, error)
		config   gh.Config
		output   []string
		wantsErr bool
	}{
		{
			name: "no authenticated hosts",
			remotes: func() (git.RemoteSet, error) {
				return git.RemoteSet{
					git.NewRemote("origin", "https://github.com/owner/repo.git"),
				}, nil
			},
			config: func() gh.Config {
				cfg := &ghmock.ConfigMock{}
				cfg.AuthenticationFunc = func() gh.AuthConfig {
					authCfg := &config.AuthConfig{}
					authCfg.SetHosts([]string{})
					authCfg.SetDefaultHost("github.com", "default")
					return authCfg
				}
				return cfg
			}(),