Repository: chainguard-dev/apko
Pull request #1379 (2024-10-30 19:00:16)

Allow multiauthenticator to try all authenticators
Previously we would fail if any authenticator returned an error. With this change, each authenticator is attempted and if any return an error the errors are aggregated and only returned if no token is successfully set.


Patch:
diff --git a/pkg/apk/auth/auth.go b/pkg/apk/auth/auth.go
index b2af1e2..915efd0 100644
--- a/pkg/apk/auth/auth.go
+++ b/pkg/apk/auth/auth.go
@@ -2,6 +2,7 @@ package auth
 
 import (
 	"context"
+	"errors"
 	"net/http"
 	"os"
 	"os/exec"
@@ -40,16 +41,23 @@ func MultiAuthenticator(auths ...Authenticator) Authenticator { return multiAuth
 type multiAuthenticator []Authenticator
 
 func (m multiAuthenticator) AddAuth(ctx context.Context, req *http.Request) error {
+	var merr error
 	for _, a := range m {
 		if _, _, ok := req.BasicAuth(); ok {
 			// The request has auth, so we can stop here.
 			return nil
 		}
 		if err := a.AddAuth(ctx, req); err != nil {
-			return err
+			merr = errors.Join(merr, err)
+			continue
 		}
 	}
-	return nil
+
+	// One last check at the end to see if we added auth, else return the aggregated error.
+	if _, _, ok := req.BasicAuth(); ok {
+		return nil
+	}
+	return merr
 }
 
 // EnvAuth adds HTTP basic auth to the request if the request URL matches the


Test patch:
diff --git a/pkg/apk/auth/auth_test.go b/pkg/apk/auth/auth_test.go
new file mode 100644
index 0000000..63fdbfb
--- /dev/null
+++ b/pkg/apk/auth/auth_test.go
@@ -0,0 +1,72 @@
+package auth
+
+import (
+	"context"
+	"errors"
+	"net/http"
+	"testing"
+)
+
+type successAuth struct{}
+
+func (s successAuth) AddAuth(_ context.Context, req *http.Request) error {
+	req.SetBasicAuth("user", "pass")
+	return nil
+}
+
+type failAuth struct{}
+
+func (f failAuth) AddAuth(_ context.Context, req *http.Request) error {
+	return errors.New("failed to add auth")
+}
+
+func TestMultiAuthenticator(t *testing.T) {
+	tests := []struct {
+		name       string
+		auths      []Authenticator
+		expectAuth bool
+		expectErr  bool
+	}{
+		{
+			name:       "success auth first",
+			auths:      []Authenticator{successAuth{}, failAuth{}},
+			expectAuth: true,
+			expectErr:  false,
+		},
+		{
+			name:       "fail auth first",
+			auths:      []Authenticator{failAuth{}, successAuth{}},
+			expectAuth: true,
+			expectErr:  false,
+		},
+		{
+			name:       "all fail auth",
+			auths:      []Authenticator{failAuth{}, failAuth{}},
+			expectAuth: false,
+			expectErr:  true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			multiAuth := MultiAuthenticator(tt.auths...)
+			req, _ := http.NewRequest("GET", "http://example.com", nil)
+			err := multiAuth.AddAuth(context.Background(), req)
+
+			if tt.expectErr && err == nil {
+				t.Errorf("expected error but got none")
+			}
+			if !tt.expectErr && err != nil {
+				t.Errorf("did not expect error but got: %v", err)
+			}
+
+			user, pass, ok := req.BasicAuth()
+			if tt.expectAuth && !ok {
+				t.Errorf("expected auth but got none")
+			}
+			if !tt.expectAuth && ok {
+				t.Errorf("did not expect auth but got user: %s, pass: %s", user, pass)
+			}
+		})
+	}
+}