From fe97423f9785a061ba7ebc7e51e1ec4014d113d7 Mon Sep 17 00:00:00 2001 From: Nish Krishnan Date: Fri, 23 Oct 2020 19:49:39 -0700 Subject: [PATCH] Ensure that refreshed creds are used to update remote. --- server/events/git_cred_writer.go | 9 ++++++- server/events/git_cred_writer_test.go | 18 ++++++++++++++ server/events/working_dir.go | 36 +++++++++++++++++++++------ server/events/working_dir_test.go | 4 +-- 4 files changed, 56 insertions(+), 11 deletions(-) diff --git a/server/events/git_cred_writer.go b/server/events/git_cred_writer.go index ec27ade0a..ab92d038c 100644 --- a/server/events/git_cred_writer.go +++ b/server/events/git_cred_writer.go @@ -105,5 +105,12 @@ func fileLineReplace(line, user, host, filename string) error { newLines = append(newLines, l) } } - return ioutil.WriteFile(filename, []byte(strings.Join(newLines, "\n")), 0600) + toWrite := strings.Join(newLines, "\n") + + // there was nothing to replace so we need to append the creds + if toWrite == "" { + return fileAppend(line, filename) + } + + return ioutil.WriteFile(filename, []byte(toWrite), 0600) } diff --git a/server/events/git_cred_writer_test.go b/server/events/git_cred_writer_test.go index de025ee51..3e881666a 100644 --- a/server/events/git_cred_writer_test.go +++ b/server/events/git_cred_writer_test.go @@ -84,6 +84,24 @@ func TestWriteGitCreds_ReplaceApp(t *testing.T) { Equals(t, expContets, string(actContents)) } +// Test that the github app credentials get updated when cred file is empty. +func TestWriteGitCreds_AppendApp(t *testing.T) { + tmp, cleanup := TempDir(t) + defer cleanup() + + credsFile := filepath.Join(tmp, ".git-credentials") + contents := "" + err := ioutil.WriteFile(credsFile, []byte(contents), 0600) + Ok(t, err) + + err = events.WriteGitCreds("x-access-token", "token", "github.com", tmp, logger, true) + Ok(t, err) + expContets := "https://x-access-token:token@github.com" + actContents, err := ioutil.ReadFile(filepath.Join(tmp, ".git-credentials")) + Ok(t, err) + Equals(t, expContets, string(actContents)) +} + // Test that if we can't read the existing file to see if the contents will be // the same that we just error out. func TestWriteGitCreds_ErrIfCannotRead(t *testing.T) { diff --git a/server/events/working_dir.go b/server/events/working_dir.go index 2a09bb53a..77125b177 100644 --- a/server/events/working_dir.go +++ b/server/events/working_dir.go @@ -104,7 +104,7 @@ func (w *FileWorkspace) Clone( // commit, only a 12 character prefix. if strings.HasPrefix(currCommit, p.HeadCommit) { log.Debug("repo is at correct commit %q so will not re-clone", p.HeadCommit) - return cloneDir, w.warnDiverged(log, cloneDir), nil + return cloneDir, w.warnDiverged(log, p, headRepo, cloneDir), nil } log.Debug("repo was already cloned but is not at correct commit, wanted %q got %q", p.HeadCommit, currCommit) @@ -122,7 +122,7 @@ func (w *FileWorkspace) Clone( // Then users won't be getting the merge functionality they expected. // If there are any errors we return false since we prefer things to succeed // vs. stopping the plan/apply. -func (w *FileWorkspace) warnDiverged(log *logging.SimpleLogger, cloneDir string) bool { +func (w *FileWorkspace) warnDiverged(log *logging.SimpleLogger, p models.PullRequest, headRepo models.Repo, cloneDir string) bool { if !w.CheckoutMerge { // It only makes sense to warn that master has diverged if we're using // the checkout merge strategy. If we're just checking out the branch, @@ -132,12 +132,32 @@ func (w *FileWorkspace) warnDiverged(log *logging.SimpleLogger, cloneDir string) } // Bring our remote refs up to date. - remoteUpdateCmd := exec.Command("git", "remote", "update") - remoteUpdateCmd.Dir = cloneDir - outputRemoteUpdate, err := remoteUpdateCmd.CombinedOutput() - if err != nil { - log.Warn("getting remote update failed: %s", string(outputRemoteUpdate)) - return false + // Reset the URL in case we are using github app credentials since these might have + // expired and refreshed and the URL would now be different. + // In this case, we should be using a proxy URL which substitutes the credentials in + // as a long term fix, but something like that requires more e2e testing/time + cmds := [][]string{ + { + "git", "remote", "set-url", "origin", p.BaseRepo.CloneURL, + }, + { + "git", "remote", "set-url", "head", headRepo.CloneURL, + }, + { + "git", "remote", "update", + }, + } + + for _, args := range cmds { + cmd := exec.Command(args[0], args[1:]...) // nolint: gosec + cmd.Dir = cloneDir + + output, err := cmd.CombinedOutput() + + if err != nil { + log.Warn("getting remote update failed: %s", string(output)) + return false + } } // Check if remote master branch has diverged. diff --git a/server/events/working_dir_test.go b/server/events/working_dir_test.go index 163b0919e..be003ae92 100644 --- a/server/events/working_dir_test.go +++ b/server/events/working_dir_test.go @@ -377,8 +377,8 @@ func TestClone_MasterHasDiverged(t *testing.T) { DataDir: repoDir, CheckoutMerge: true, } - _, hasDiverged, err := wd.Clone(nil, models.Repo{}, models.PullRequest{ - BaseRepo: models.Repo{}, + _, hasDiverged, err := wd.Clone(nil, models.Repo{CloneURL: repoDir}, models.PullRequest{ + BaseRepo: models.Repo{CloneURL: repoDir}, HeadBranch: "second-pr", BaseBranch: "master", }, "default")