From 4f7b93a01004fe11504ee67556365ca792441453 Mon Sep 17 00:00:00 2001 From: Terry Howe Date: Thu, 11 Sep 2025 10:23:49 -0600 Subject: [PATCH] chore: handle mutliple threads Signed-off-by: Terry Howe --- pkg/registry/authorizer.go | 40 +++++++++++++++++---- pkg/registry/authorizer_test.go | 64 ++++++++++++++++++++++++++++++--- 2 files changed, 92 insertions(+), 12 deletions(-) diff --git a/pkg/registry/authorizer.go b/pkg/registry/authorizer.go index 53e41587a..cbae9df98 100644 --- a/pkg/registry/authorizer.go +++ b/pkg/registry/authorizer.go @@ -20,6 +20,7 @@ import ( "context" "net/http" "strings" + "sync" "helm.sh/helm/v4/internal/version" @@ -29,7 +30,8 @@ import ( type Authorizer struct { auth.Client - AttemptBearerAuthentication bool + lock sync.RWMutex + attemptBearerAuthentication bool } func NewAuthorizer(httpClient *http.Client, credentialsStore credentials.Store, username, password string) *Authorizer { @@ -48,7 +50,7 @@ func NewAuthorizer(httpClient *http.Client, credentialsStore credentials.Store, authorizer.Credential = credentials.Credential(credentialsStore) } - authorizer.AttemptBearerAuthentication = true + authorizer.setAttemptBearerAuthentication(true) return &authorizer } @@ -56,19 +58,43 @@ func (a *Authorizer) EnableCache() { a.Cache = auth.NewCache() } +func (a *Authorizer) getAttemptBearerAuthentication() bool { + a.lock.RLock() + defer a.lock.RUnlock() + return a.attemptBearerAuthentication +} + +func (a *Authorizer) setAttemptBearerAuthentication(value bool) { + a.lock.Lock() + defer a.lock.Unlock() + a.attemptBearerAuthentication = value +} + +func (a *Authorizer) getForceAttemptOAuth2() bool { + a.lock.RLock() + defer a.lock.RUnlock() + return a.ForceAttemptOAuth2 +} + +func (a *Authorizer) setForceAttemptOAuth2(value bool) { + a.lock.Lock() + defer a.lock.Unlock() + a.ForceAttemptOAuth2 = value +} + // Do This method wraps auth.Client.Do in attempt to retry authentication func (a *Authorizer) Do(originalReq *http.Request) (*http.Response, error) { - if a.AttemptBearerAuthentication { + if a.getAttemptBearerAuthentication() { needsAuthentication := originalReq.Header.Get("Authorization") == "" if needsAuthentication { - a.ForceAttemptOAuth2 = true + a.setForceAttemptOAuth2(true) if originalReq.Host == "ghcr.io" { - a.ForceAttemptOAuth2 = false - a.AttemptBearerAuthentication = false + a.setForceAttemptOAuth2(false) + a.setAttemptBearerAuthentication(false) } resp, err := a.Client.Do(originalReq) if err == nil { - a.AttemptBearerAuthentication = false + a.setAttemptBearerAuthentication(false) return resp, nil } if !strings.Contains(err.Error(), "response status code 40") { diff --git a/pkg/registry/authorizer_test.go b/pkg/registry/authorizer_test.go index a084c8b02..d0267803b 100644 --- a/pkg/registry/authorizer_test.go +++ b/pkg/registry/authorizer_test.go @@ -20,6 +20,7 @@ import ( "context" "net/http" "net/http/httptest" + "sync" "testing" "github.com/stretchr/testify/assert" @@ -78,7 +79,7 @@ func TestNewAuthorizer(t *testing.T) { require.NotNil(t, authorizer) assert.Equal(t, httpClient, authorizer.Client.Client) - assert.True(t, authorizer.AttemptBearerAuthentication) + assert.True(t, authorizer.getAttemptBearerAuthentication()) assert.NotNil(t, authorizer.Credential) if tt.username != "" && tt.password != "" { @@ -179,10 +180,10 @@ func TestAuthorizer_Do(t *testing.T) { require.NoError(t, err) require.NotNil(t, resp) - assert.Equal(t, tt.expectBearerAuthAfter, authorizer.AttemptBearerAuthentication) + assert.Equal(t, tt.expectBearerAuthAfter, authorizer.getAttemptBearerAuthentication()) if tt.authHeader == "" { - assert.Equal(t, tt.expectForceOAuth2, authorizer.ForceAttemptOAuth2) + assert.Equal(t, tt.expectForceOAuth2, authorizer.getForceAttemptOAuth2()) } resp.Body.Close() @@ -201,7 +202,7 @@ func TestAuthorizer_Do_WithBearerAttemptDisabled(t *testing.T) { credStore := &mockCredentialsStore{} authorizer := NewAuthorizer(httpClient, credStore, "", "") - authorizer.AttemptBearerAuthentication = false + authorizer.setAttemptBearerAuthentication(false) req, err := http.NewRequest(http.MethodGet, server.URL, nil) require.NoError(t, err) @@ -212,7 +213,7 @@ func TestAuthorizer_Do_WithBearerAttemptDisabled(t *testing.T) { require.NoError(t, err) require.NotNil(t, resp) assert.Equal(t, http.StatusOK, resp.StatusCode) - assert.False(t, authorizer.AttemptBearerAuthentication) + assert.False(t, authorizer.getAttemptBearerAuthentication()) resp.Body.Close() } @@ -241,3 +242,56 @@ func TestAuthorizer_Do_NonRetryableError(t *testing.T) { resp.Body.Close() } + +func TestAuthorizer_ConcurrentAccess(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte("success")) + })) + defer server.Close() + + httpClient := &http.Client{} + credStore := &mockCredentialsStore{} + authorizer := NewAuthorizer(httpClient, credStore, "", "") + + const numGoroutines = 100 + const numRequests = 10 + + var wg sync.WaitGroup + wg.Add(numGoroutines * 2) + + for i := 0; i < numGoroutines; i++ { + go func() { + defer wg.Done() + for j := 0; j < numRequests; j++ { + req, err := http.NewRequest(http.MethodGet, server.URL, nil) + require.NoError(t, err) + req.Host = "registry.example.com" + + resp, err := authorizer.Do(req) + if err == nil && resp != nil { + resp.Body.Close() + } + } + }() + + go func() { + defer wg.Done() + for j := 0; j < numRequests; j++ { + authorizer.setAttemptBearerAuthentication(true) + val := authorizer.getAttemptBearerAuthentication() + if val != true { + t.Logf("Warning: Expected true but got %v", val) + } + + authorizer.setAttemptBearerAuthentication(false) + val = authorizer.getAttemptBearerAuthentication() + if val != false { + t.Logf("Warning: Expected false but got %v", val) + } + } + }() + } + + wg.Wait() +}