From ba1afa2c5d03d252b6717bc6a8e2b64f68de4b60 Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 17 May 2024 14:16:14 +0200 Subject: [PATCH] Add HelperConfig contract test and FakeHelperConfig --- pkg/cmd/auth/shared/contract/helper_config.go | 59 +++++++++ pkg/cmd/auth/shared/git_credential.go | 5 + .../gitcredentials/fake_helper_config.go | 49 ++++++++ .../gitcredentials/fake_helper_config_test.go | 32 +++++ .../gitcredentials/helper_config_test.go | 119 +++++------------- 5 files changed, 173 insertions(+), 91 deletions(-) create mode 100644 pkg/cmd/auth/shared/contract/helper_config.go create mode 100644 pkg/cmd/auth/shared/gitcredentials/fake_helper_config.go create mode 100644 pkg/cmd/auth/shared/gitcredentials/fake_helper_config_test.go diff --git a/pkg/cmd/auth/shared/contract/helper_config.go b/pkg/cmd/auth/shared/contract/helper_config.go new file mode 100644 index 000000000..287eb7c90 --- /dev/null +++ b/pkg/cmd/auth/shared/contract/helper_config.go @@ -0,0 +1,59 @@ +package contract + +import ( + "testing" + + "github.com/cli/cli/v2/pkg/cmd/auth/shared" + "github.com/stretchr/testify/require" +) + +type HelperConfig struct { + NewHelperConfig func(t *testing.T) shared.HelperConfig + ConfigureHelper func(t *testing.T, hostname string) +} + +func (contract HelperConfig) Test(t *testing.T) { + t.Run("when there are no credential helpers, configures gh for repo and gist host", func(t *testing.T) { + hc := contract.NewHelperConfig(t) + require.NoError(t, hc.ConfigureOurs("github.com")) + + repoHelper, err := hc.ConfiguredHelper("github.com") + require.NoError(t, err) + require.True(t, repoHelper.IsConfigured(), "expected our helper to be configured") + require.True(t, repoHelper.IsOurs(), "expected the helper to be ours but was %q", repoHelper.Cmd) + + gistHelper, err := hc.ConfiguredHelper("gist.github.com") + require.NoError(t, err) + require.True(t, gistHelper.IsConfigured(), "expected our helper to be configured") + require.True(t, gistHelper.IsOurs(), "expected the helper to be ours but was %q", gistHelper.Cmd) + }) + + t.Run("when there is a global credential helper, it should be configured but not ours", func(t *testing.T) { + hc := contract.NewHelperConfig(t) + contract.ConfigureHelper(t, "credential.helper") + + helper, err := hc.ConfiguredHelper("github.com") + require.NoError(t, err) + require.True(t, helper.IsConfigured(), "expected helper to be configured") + require.False(t, helper.IsOurs(), "expected the helper not to be ours but was %q", helper.Cmd) + }) + + t.Run("when there is a host credential helper, it should be configured but not ours", func(t *testing.T) { + hc := contract.NewHelperConfig(t) + contract.ConfigureHelper(t, "credential.https://github.com.helper") + + helper, err := hc.ConfiguredHelper("github.com") + require.NoError(t, err) + require.True(t, helper.IsConfigured(), "expected helper to be configured") + require.False(t, helper.IsOurs(), "expected the helper not to be ours but was %q", helper.Cmd) + }) + + t.Run("returns non configured helper when no helpers are configured", func(t *testing.T) { + hc := contract.NewHelperConfig(t) + + helper, err := hc.ConfiguredHelper("github.com") + require.NoError(t, err) + require.False(t, helper.IsConfigured(), "expected no helper to be configured") + require.False(t, helper.IsOurs(), "expected the helper not to be ours but was %q", helper.Cmd) + }) +} diff --git a/pkg/cmd/auth/shared/git_credential.go b/pkg/cmd/auth/shared/git_credential.go index c98992a76..ad552730e 100644 --- a/pkg/cmd/auth/shared/git_credential.go +++ b/pkg/cmd/auth/shared/git_credential.go @@ -7,6 +7,11 @@ import ( "github.com/cli/cli/v2/pkg/cmd/auth/shared/gitcredentials" ) +type HelperConfig interface { + ConfigureOurs(hostname string) error + ConfiguredHelper(hostname string) (gitcredentials.Helper, error) +} + type GitCredentialFlow struct { Prompter Prompt diff --git a/pkg/cmd/auth/shared/gitcredentials/fake_helper_config.go b/pkg/cmd/auth/shared/gitcredentials/fake_helper_config.go new file mode 100644 index 000000000..f6ae2f2c0 --- /dev/null +++ b/pkg/cmd/auth/shared/gitcredentials/fake_helper_config.go @@ -0,0 +1,49 @@ +package gitcredentials + +import ( + "fmt" + "strings" + + "github.com/cli/cli/v2/internal/ghinstance" +) + +type FakeHelperConfig struct { + SelfExecutablePath string + Helpers map[string]Helper +} + +// ConfigureOurs sets up the git credential helper chain to use the GitHub CLI credential helper for git repositories +// including gists. +func (hc *FakeHelperConfig) ConfigureOurs(hostname string) error { + credHelperKeys := []string{ + keyFor(hostname), + } + + gistHost := strings.TrimSuffix(ghinstance.GistHost(hostname), "/") + if strings.HasPrefix(gistHost, "gist.") { + credHelperKeys = append(credHelperKeys, keyFor(gistHost)) + } + + for _, credHelperKey := range credHelperKeys { + hc.Helpers[credHelperKey] = Helper{ + Cmd: fmt.Sprintf("!%s auth git-credential", shellQuote(hc.SelfExecutablePath)), + } + } + + return nil +} + +// ConfiguredHelper returns the configured git credential helper for a given hostname. +func (hc *FakeHelperConfig) ConfiguredHelper(hostname string) (Helper, error) { + helper, ok := hc.Helpers[keyFor(hostname)] + if ok { + return helper, nil + } + + helper, ok = hc.Helpers["credential.helper"] + if ok { + return helper, nil + } + + return Helper{}, nil +} diff --git a/pkg/cmd/auth/shared/gitcredentials/fake_helper_config_test.go b/pkg/cmd/auth/shared/gitcredentials/fake_helper_config_test.go new file mode 100644 index 000000000..441972aff --- /dev/null +++ b/pkg/cmd/auth/shared/gitcredentials/fake_helper_config_test.go @@ -0,0 +1,32 @@ +package gitcredentials_test + +import ( + "testing" + + "github.com/cli/cli/v2/pkg/cmd/auth/shared" + "github.com/cli/cli/v2/pkg/cmd/auth/shared/contract" + "github.com/cli/cli/v2/pkg/cmd/auth/shared/gitcredentials" +) + +func TestFakeHelperConfigContract(t *testing.T) { + // Note that this being mutated by `NewHelperConfig` makes these tests not parallelizable + var fhc *gitcredentials.FakeHelperConfig + + contract.HelperConfig{ + NewHelperConfig: func(t *testing.T) shared.HelperConfig { + // Mutate the closed over fhc so that ConfigureHelper is able to configure helpers + // for tests. An alternative would be to provide the Helper as an argument back to ConfigureHelper + // but then we'd have to type assert it back to *FakeHelperConfig, which is probably more trouble than + // it's worth to parallelize these tests, sinced it's not even possible to parallelize the real Helperconfig + // ones due to them using t.Setenv + fhc = &gitcredentials.FakeHelperConfig{ + SelfExecutablePath: "/path/to/gh", + Helpers: map[string]gitcredentials.Helper{}, + } + return fhc + }, + ConfigureHelper: func(t *testing.T, hostname string) { + fhc.Helpers[hostname] = gitcredentials.Helper{Cmd: "test-helper"} + }, + }.Test(t) +} diff --git a/pkg/cmd/auth/shared/gitcredentials/helper_config_test.go b/pkg/cmd/auth/shared/gitcredentials/helper_config_test.go index b7ddbb883..80ffac85a 100644 --- a/pkg/cmd/auth/shared/gitcredentials/helper_config_test.go +++ b/pkg/cmd/auth/shared/gitcredentials/helper_config_test.go @@ -7,6 +7,8 @@ import ( "testing" "github.com/cli/cli/v2/git" + "github.com/cli/cli/v2/pkg/cmd/auth/shared" + "github.com/cli/cli/v2/pkg/cmd/auth/shared/contract" "github.com/cli/cli/v2/pkg/cmd/auth/shared/gitcredentials" "github.com/stretchr/testify/require" ) @@ -33,105 +35,40 @@ func configureTestCredentialHelper(t *testing.T, key string) { require.NoError(t, cmd.Run()) } -func TestConfigureOursNoPreexistingHelpersConfigured(t *testing.T) { - // Given there are no credential helpers configured +func TestHelperConfigContract(t *testing.T) { + contract.HelperConfig{ + NewHelperConfig: func(t *testing.T) shared.HelperConfig { + withIsolatedGitConfig(t) + + return &gitcredentials.HelperConfig{ + SelfExecutablePath: "/path/to/gh", + GitClient: &git.Client{}, + } + }, + ConfigureHelper: func(t *testing.T, hostname string) { + configureTestCredentialHelper(t, hostname) + }, + }.Test(t) +} + +// This is a whitebox test unlike the contract because although we don't use the exact configured command, it's +// important that it is exactly right since git uses it. +func TestSetsCorrectCommandInGitConfig(t *testing.T) { withIsolatedGitConfig(t) - // When we configure ourselves as the git credential helper - hc := gitcredentials.HelperConfig{ + gc := &git.Client{} + hc := &gitcredentials.HelperConfig{ SelfExecutablePath: "/path/to/gh", - GitClient: &git.Client{}, + GitClient: gc, } require.NoError(t, hc.ConfigureOurs("github.com")) - // Then the git config should be updated to use our credential helper for both repos and gists - repoHelper, err := hc.ConfiguredHelper("github.com") + // Check that the correct command was set in the git config + cmd, err := gc.Command(context.Background(), "config", "--get", "credential.https://github.com.helper") require.NoError(t, err) - require.True(t, repoHelper.IsConfigured(), "expected our helper to be configured") - require.True(t, repoHelper.IsOurs(), "expected the helper to be ours but was %q", repoHelper.Cmd) - - gistHelper, err := hc.ConfiguredHelper("gist.github.com") + output, err := cmd.Output() require.NoError(t, err) - require.True(t, gistHelper.IsConfigured(), "expected our helper to be configured") - require.True(t, gistHelper.IsOurs(), "expected the helper to be ours but was %q", gistHelper.Cmd) -} - -func TestConfigureOursWithPreexistingHelpersConfigured(t *testing.T) { - // Given there are other credential helpers configured - withIsolatedGitConfig(t) - configureTestCredentialHelper(t, "credential.helper") - configureTestCredentialHelper(t, "credential.https://github.com.helper") - - // When we configure ourselves as the git credential helper - hc := gitcredentials.HelperConfig{ - SelfExecutablePath: "/path/to/gh", - GitClient: &git.Client{}, - } - require.NoError(t, hc.ConfigureOurs("github.com")) - - // Then the git config should be updated to use our credential repoHelper for both repos and gists - repoHelper, err := hc.ConfiguredHelper("github.com") - require.NoError(t, err) - require.True(t, repoHelper.IsConfigured(), "expected our helper to be configured") - require.True(t, repoHelper.IsOurs(), "expected the helper to be ours but was %q", repoHelper.Cmd) - - gistHelper, err := hc.ConfiguredHelper("gist.github.com") - require.NoError(t, err) - require.True(t, gistHelper.IsConfigured(), "expected our helper to be configured") - require.True(t, gistHelper.IsOurs(), "expected the helper to be ours but was %q", gistHelper.Cmd) -} - -func TestConfiguredHelperNoPreexistingHelpersConfigured(t *testing.T) { - // Given there are no credential helpers configured - withIsolatedGitConfig(t) - - // When we check the configured helper for a hostname - hc := gitcredentials.HelperConfig{ - SelfExecutablePath: "/path/to/gh", - GitClient: &git.Client{}, - } - helper, err := hc.ConfiguredHelper("github.com") - - // Then the helper should not be configured and not ours - require.NoError(t, err) - require.False(t, helper.IsConfigured(), "expected no helper to be configured") - require.False(t, helper.IsOurs(), "expected the helper not to be ours but was %q", helper.Cmd) -} - -func TestConfiguredHelperWithPreexistingGlobalHelperConfigured(t *testing.T) { - // Given there is a global credential helper configured - withIsolatedGitConfig(t) - configureTestCredentialHelper(t, "credential.helper") - - // When we check the configured helper for a hostname - hc := gitcredentials.HelperConfig{ - SelfExecutablePath: "/path/to/gh", - GitClient: &git.Client{}, - } - helper, err := hc.ConfiguredHelper("github.com") - - // Then the helper should be configured, but not ours - require.NoError(t, err) - require.True(t, helper.IsConfigured(), "expected no helper to be configured") - require.False(t, helper.IsOurs(), "expected the helper not to be ours but was %q", helper.Cmd) -} - -func TestConfiguredHelperWithPreexistingHostHelperConfigured(t *testing.T) { - // Given there is a credential helper configured for a hostname - withIsolatedGitConfig(t) - configureTestCredentialHelper(t, "credential.https://github.com.helper") - - // When we check the configured helper for a hostname - hc := gitcredentials.HelperConfig{ - SelfExecutablePath: "/path/to/gh", - GitClient: &git.Client{}, - } - helper, err := hc.ConfiguredHelper("github.com") - - // Then the helper should be configured, but not ours - require.NoError(t, err) - require.True(t, helper.IsConfigured(), "expected no helper to be configured") - require.False(t, helper.IsOurs(), "expected the helper not to be ours but was %q", helper.Cmd) + require.Equal(t, "!/path/to/gh auth git-credential\n", string(output)) } func TestHelperIsOurs(t *testing.T) {