From dd57eb00496dca325d68bbd898f721e9069f4fdb Mon Sep 17 00:00:00 2001
From: Karl Isenberg <karlisenberg@google.com>
Date: Thu, 20 Jul 2023 17:17:49 -0700
Subject: [PATCH] Handle errors from credential refresh

Previously, errors from askpass and credential storage were being
ignored, causing git clone/fetch to later error with hard-to-read
errors.

Now the error indicates the credential refresh as the problem, and
does not try to sync.
---
 main.go     |   8 ++--
 test_e2e.sh | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 109 insertions(+), 6 deletions(-)

diff --git a/main.go b/main.go
index 8ddb7e9..cdb18d4 100644
--- a/main.go
+++ b/main.go
@@ -944,8 +944,8 @@ func main() {
 			}
 		}
 		if *flAskPassURL != "" {
-			// When using an auth URL, the credentials can be dynamic, it needs to be
-			// re-fetched each time.
+			// When using an auth URL, the credentials can be dynamic, and need
+			// to be re-fetched each time.
 			if err := git.CallAskPassURL(ctx); err != nil {
 				metricAskpassCount.WithLabelValues(metricKeyError).Inc()
 				return err
@@ -1723,7 +1723,9 @@ func (git *repoSync) currentWorktree() (worktree, error) {
 func (git *repoSync) SyncRepo(ctx context.Context, refreshCreds func(context.Context) error) (bool, string, error) {
 	git.log.V(3).Info("syncing", "repo", git.repo)
 
-	refreshCreds(ctx)
+	if err := refreshCreds(ctx); err != nil {
+		return false, "", fmt.Errorf("credential refresh failed: %w", err)
+	}
 
 	// Initialize the repo directory if needed.
 	if err := git.initRepo(ctx); err != nil {
diff --git a/test_e2e.sh b/test_e2e.sh
index 29646a1..14c7c90 100755
--- a/test_e2e.sh
+++ b/test_e2e.sh
@@ -1763,9 +1763,9 @@ function e2e::auth_askpass_url_correct_password() {
 }
 
 ##############################################
-# Test askpass-url where the URL is flaky
+# Test askpass-url where the URL is sometimes wrong
 ##############################################
-function e2e::auth_askpass_url_flaky() {
+function e2e::auth_askpass_url_sometimes_wrong() {
     # run with askpass_url service which alternates good/bad replies.
     HITLOG="$WORK/hitlog"
     cat /dev/null > "$HITLOG"
@@ -1777,7 +1777,7 @@ function e2e::auth_askpass_url_flaky() {
             echo
             if [ -f /tmp/flag ]; then
                 echo "username=my-username"
-               echo "password=my-password"
+                echo "password=my-password"
                 rm /tmp/flag
             else
                 echo "username=my-username"
@@ -1821,6 +1821,107 @@ function e2e::auth_askpass_url_flaky() {
     assert_file_eq "$ROOT/link/file" "$FUNCNAME 1"
 }
 
+##############################################
+# Test askpass-url where the URL is flaky
+##############################################
+function e2e::auth_askpass_url_flaky() {
+    # run with askpass_url service which alternates good/bad replies.
+    HITLOG="$WORK/hitlog"
+    cat /dev/null > "$HITLOG"
+    CTR=$(docker_run \
+        -v "$HITLOG":/var/log/hits \
+        e2e/test/ncsvr \
+        80 'read X
+            if [ -f /tmp/flag ]; then
+                echo "HTTP/1.1 200 OK"
+                echo
+                echo "username=my-username"
+                echo "password=my-password"
+                rm /tmp/flag
+            else
+                echo "HTTP/1.1 503 Service Unavailable"
+                echo
+                touch /tmp/flag
+            fi
+            ')
+    IP=$(docker_ip "$CTR")
+
+    # First sync
+    echo "$FUNCNAME 1" > "$REPO/file"
+    git -C "$REPO" commit -qam "$FUNCNAME 1"
+
+    GIT_SYNC \
+        --git="/$ASKPASS_GIT" \
+        --askpass-url="http://$IP/git_askpass" \
+        --max-failures=2 \
+        --period=100ms \
+        --repo="file://$REPO" \
+        --root="$ROOT" \
+        --link="link" \
+        &
+    wait_for_sync "${MAXWAIT}"
+    assert_link_exists "$ROOT/link"
+    assert_file_exists "$ROOT/link/file"
+    assert_file_eq "$ROOT/link/file" "$FUNCNAME 1"
+
+    # Move HEAD forward
+    echo "$FUNCNAME 2" > "$REPO/file"
+    git -C "$REPO" commit -qam "$FUNCNAME 2"
+    wait_for_sync "${MAXWAIT}"
+    assert_link_exists "$ROOT/link"
+    assert_file_exists "$ROOT/link/file"
+    assert_file_eq "$ROOT/link/file" "$FUNCNAME 2"
+
+    # Move HEAD backward
+    git -C "$REPO" reset -q --hard HEAD^
+    wait_for_sync "${MAXWAIT}"
+    assert_link_exists "$ROOT/link"
+    assert_file_exists "$ROOT/link/file"
+    assert_file_eq "$ROOT/link/file" "$FUNCNAME 1"
+}
+
+##############################################
+# Test askpass-url where the URL fails at startup
+##############################################
+function e2e::auth_askpass_url_slow_start() {
+    # run with askpass_url service which takes a while to come up
+    HITLOG="$WORK/hitlog"
+    cat /dev/null > "$HITLOG"
+    CTR=$(docker_run \
+        -v "$HITLOG":/var/log/hits \
+        --entrypoint sh \
+        e2e/test/ncsvr \
+        -c "sleep 4;
+            /ncsvr.sh 80 'read X
+                echo \"HTTP/1.1 200 OK\"
+                echo
+                echo \"username=my-username\"
+                echo \"password=my-password\"
+                '")
+    IP=$(docker_ip "$CTR")
+
+    # Sync
+    echo "$FUNCNAME" > "$REPO/file"
+    git -C "$REPO" commit -qam "$FUNCNAME"
+
+    GIT_SYNC \
+        --git="/$ASKPASS_GIT" \
+        --askpass-url="http://$IP/git_askpass" \
+        --max-failures=5 \
+        --period=1s \
+        --repo="file://$REPO" \
+        --root="$ROOT" \
+        --link="link" \
+        &
+    sleep 1
+    assert_file_absent "$ROOT/link"
+
+    wait_for_sync "5"
+    assert_link_exists "$ROOT/link"
+    assert_file_exists "$ROOT/link/file"
+    assert_file_eq "$ROOT/link/file" "$FUNCNAME"
+}
+
 ##############################################
 # Test exechook-success
 ##############################################