Repository: go-acme/lego
Pull request #2445 (2025-02-17 15:45:42)

acme-dns: use new registred account
Related to https://github.com/go-acme/lego/pull/2443#issuecomment-2663470866

dmke: I think this would need a proper test case, no? This is the third change related to account creation in short time due to missed edge cases...
skippyprime: This tests well locally.  Thank you for all of your support!
ldez: > I think this would need a proper test case, no?

I'm currently writing tests, but I wanted to open the PR quickly.

Patch:
diff --git a/providers/dns/acmedns/acmedns.go b/providers/dns/acmedns/acmedns.go
index 15e0a1da..a5e5a407 100644
--- a/providers/dns/acmedns/acmedns.go
+++ b/providers/dns/acmedns/acmedns.go
@@ -178,7 +178,7 @@ func (d *DNSProvider) Present(domain, _, keyAuth string) error {
 
 		// The account did not exist.
 		// Create a new one and return an error indicating the required one-time manual CNAME setup.
-		err = d.register(ctx, domain, info.FQDN)
+		account, err = d.register(ctx, domain, info.FQDN)
 		if err != nil {
 			return err
 		}
@@ -200,10 +200,10 @@ func (d *DNSProvider) CleanUp(_, _, _ string) error {
 // If account creation works as expected a ErrCNAMERequired error is returned describing
 // the one-time manual CNAME setup required to complete setup of the ACME-DNS hook for the domain.
 // If any other error occurs it is returned as-is.
-func (d *DNSProvider) register(ctx context.Context, domain, fqdn string) error {
+func (d *DNSProvider) register(ctx context.Context, domain, fqdn string) (goacmedns.Account, error) {
 	newAcct, err := d.client.RegisterAccount(ctx, d.config.AllowList)
 	if err != nil {
-		return err
+		return goacmedns.Account{}, err
 	}
 
 	var cnameCreated bool
@@ -213,23 +213,23 @@ func (d *DNSProvider) register(ctx context.Context, domain, fqdn string) error {
 	if err != nil {
 		cnameCreated = errors.Is(err, internal.ErrCNAMEAlreadyCreated)
 		if !cnameCreated {
-			return err
+			return goacmedns.Account{}, err
 		}
 	}
 
 	err = d.storage.Save(ctx)
 	if err != nil {
-		return err
+		return goacmedns.Account{}, err
 	}
 
 	if cnameCreated {
-		return nil
+		return newAcct, nil
 	}
 
 	// Stop issuance by returning an error.
 	// The user needs to perform a manual one-time CNAME setup in their DNS zone
 	// to complete the setup of the new account we created.
-	return ErrCNAMERequired{
+	return goacmedns.Account{}, ErrCNAMERequired{
 		Domain: domain,
 		FQDN:   fqdn,
 		Target: newAcct.FullDomain,


Test patch:
diff --git a/providers/dns/acmedns/acmedns_test.go b/providers/dns/acmedns/acmedns_test.go
index 76aa097f..081080d2 100644
--- a/providers/dns/acmedns/acmedns_test.go
+++ b/providers/dns/acmedns/acmedns_test.go
@@ -2,6 +2,8 @@ package acmedns
 
 import (
 	"context"
+	"net/http"
+	"net/http/httptest"
 	"testing"
 
 	"github.com/nrdcg/goacmedns"
@@ -10,27 +12,17 @@ import (
 )
 
 const (
-	// Fixed test data for unit tests.
 	egDomain  = "example.com"
 	egFQDN    = "_acme-challenge." + egDomain + "."
 	egKeyAuth = "⚷"
 )
 
-// TestPresent tests that the ACME-DNS Present function for updating a DNS-01
-// challenge response TXT record works as expected.
 func TestPresent(t *testing.T) {
 	// validAccountStorage is a mockStorage configured to return the egTestAccount.
-	validAccountStorage := mockStorage{
-		map[string]goacmedns.Account{
-			egDomain: egTestAccount,
-		},
-	}
-	// validUpdateClient is a mockClient configured with the egTestAccount that will
-	// track TXT updates in a map.
-	validUpdateClient := mockUpdateClient{
-		mockClient{egTestAccount},
-		make(map[goacmedns.Account]string),
-	}
+	validAccountStorage := newMockStorage().WithAccount(egDomain, egTestAccount)
+
+	// validUpdateClient is a mockClient configured with the egTestAccount that will track TXT updates in a map.
+	validUpdateClient := newMockClient()
 
 	testCases := []struct {
 		Name          string
@@ -40,13 +32,13 @@ func TestPresent(t *testing.T) {
 	}{
 		{
 			Name:          "present when client storage returns unexpected error",
-			Client:        mockClient{egTestAccount},
-			Storage:       errorFetchStorage{},
+			Client:        newMockClient().WithRegisterAccount(egTestAccount),
+			Storage:       newMockStorage().WithFetchError(errorStorageErr),
 			ExpectedError: errorStorageErr,
 		},
 		{
 			Name:   "present when client storage returns ErrDomainNotFound",
-			Client: mockClient{egTestAccount},
+			Client: newMockClient().WithRegisterAccount(egTestAccount),
 			ExpectedError: ErrCNAMERequired{
 				Domain: egDomain,
 				FQDN:   egFQDN,
@@ -55,7 +47,7 @@ func TestPresent(t *testing.T) {
 		},
 		{
 			Name:          "present when client UpdateTXTRecord returns unexpected error",
-			Client:        errorUpdateClient{},
+			Client:        newMockClient().WithUpdateTXTRecordError(errorClientErr),
 			Storage:       validAccountStorage,
 			ExpectedError: errorClientErr,
 		},
@@ -71,16 +63,13 @@ func TestPresent(t *testing.T) {
 			p := &DNSProvider{
 				config:  NewDefaultConfig(),
 				client:  test.Client,
-				storage: mockStorage{make(map[string]goacmedns.Account)},
+				storage: newMockStorage(),
 			}
 
-			// override the storage mock if required by the test case.
 			if test.Storage != nil {
 				p.storage = test.Storage
 			}
 
-			// call Present. The token argument can be garbage because the ACME-DNS
-			// provider does not use it.
 			err := p.Present(egDomain, "foo", egKeyAuth)
 			if test.ExpectedError != nil {
 				assert.Equal(t, test.ExpectedError, err)
@@ -97,36 +86,33 @@ func TestPresent(t *testing.T) {
 	assert.Len(t, validUpdateClient.records[egTestAccount], 43)
 }
 
-// TestRegister tests that the ACME-DNS register function works correctly.
 func TestRegister(t *testing.T) {
 	testCases := []struct {
 		Name          string
 		Client        acmeDNSClient
 		Storage       goacmedns.Storage
-		Domain        string
-		FQDN          string
 		ExpectedError error
 	}{
 		{
 			Name:          "register when acme-dns client returns an error",
-			Client:        errorRegisterClient{},
+			Client:        newMockClient().WithRegisterAccountError(errorClientErr),
 			ExpectedError: errorClientErr,
 		},
 		{
 			Name:          "register when acme-dns storage put returns an error",
-			Client:        mockClient{egTestAccount},
-			Storage:       errorPutStorage{mockStorage{make(map[string]goacmedns.Account)}},
+			Client:        newMockClient().WithRegisterAccount(egTestAccount),
+			Storage:       newMockStorage().WithPutError(errorStorageErr),
 			ExpectedError: errorStorageErr,
 		},
 		{
 			Name:          "register when acme-dns storage save returns an error",
-			Client:        mockClient{egTestAccount},
-			Storage:       errorSaveStorage{mockStorage{make(map[string]goacmedns.Account)}},
+			Client:        newMockClient().WithRegisterAccount(egTestAccount),
+			Storage:       newMockStorage().WithSaveError(errorStorageErr),
 			ExpectedError: errorStorageErr,
 		},
 		{
 			Name:   "register when everything works",
-			Client: mockClient{egTestAccount},
+			Client: newMockClient().WithRegisterAccount(egTestAccount),
 			ExpectedError: ErrCNAMERequired{
 				Domain: egDomain,
 				FQDN:   egFQDN,
@@ -140,20 +126,128 @@ func TestRegister(t *testing.T) {
 			p := &DNSProvider{
 				config:  NewDefaultConfig(),
 				client:  test.Client,
-				storage: mockStorage{make(map[string]goacmedns.Account)},
+				storage: newMockStorage(),
 			}
 
-			// override the storage mock if required by the testcase.
 			if test.Storage != nil {
 				p.storage = test.Storage
 			}
 
-			// Call register for the example domain/fqdn.
-			err := p.register(context.Background(), egDomain, egFQDN)
+			acc, err := p.register(context.Background(), egDomain, egFQDN)
+			if test.ExpectedError != nil {
+				assert.Equal(t, test.ExpectedError, err)
+			} else {
+				assert.Equal(t, goacmedns.Account{}, acc)
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestPresent_httpStorage(t *testing.T) {
+	testCases := []struct {
+		desc          string
+		StatusCode    int
+		ExpectedError error
+	}{
+		{
+			desc:       "the CNAME is not handled by the storage",
+			StatusCode: http.StatusOK,
+			ExpectedError: ErrCNAMERequired{
+				Domain: egDomain,
+				FQDN:   egFQDN,
+				Target: egTestAccount.FullDomain,
+			},
+		},
+		{
+			desc:       "the CNAME is handled by the storage",
+			StatusCode: http.StatusCreated,
+		},
+	}
+
+	for _, test := range testCases {
+		t.Run(test.desc, func(t *testing.T) {
+			mux := http.NewServeMux()
+			server := httptest.NewServer(mux)
+
+			config := NewDefaultConfig()
+			config.StorageBaseURL = server.URL
+
+			p, err := NewDNSProviderConfig(config)
+			require.NoError(t, err)
+
+			client := newMockClient().WithRegisterAccount(egTestAccount)
+			p.client = client
+
+			// Fetch
+			mux.HandleFunc("GET /example.com", func(rw http.ResponseWriter, reg *http.Request) {
+				rw.WriteHeader(http.StatusNotFound)
+			})
+
+			// Put
+			mux.HandleFunc("POST /example.com", func(rw http.ResponseWriter, req *http.Request) {
+				rw.WriteHeader(test.StatusCode)
+			})
+
+			err = p.Present(egDomain, "foo", egKeyAuth)
+			if test.ExpectedError != nil {
+				assert.Equal(t, test.ExpectedError, err)
+				assert.True(t, client.registerAccountCalled)
+				assert.False(t, client.updateTXTRecordCalled)
+			} else {
+				require.NoError(t, err)
+				assert.True(t, client.registerAccountCalled)
+				assert.True(t, client.updateTXTRecordCalled)
+			}
+		})
+	}
+}
+
+func TestRegister_httpStorage(t *testing.T) {
+	testCases := []struct {
+		Name          string
+		StatusCode    int
+		ExpectedError error
+	}{
+		{
+			Name:       "status code 200",
+			StatusCode: http.StatusOK,
+			ExpectedError: ErrCNAMERequired{
+				Domain: egDomain,
+				FQDN:   egFQDN,
+				Target: egTestAccount.FullDomain,
+			},
+		},
+		{
+			Name:       "status code 201",
+			StatusCode: http.StatusCreated,
+		},
+	}
+
+	for _, test := range testCases {
+		t.Run(test.Name, func(t *testing.T) {
+			mux := http.NewServeMux()
+			server := httptest.NewServer(mux)
+
+			config := NewDefaultConfig()
+			config.StorageBaseURL = server.URL
+
+			p, err := NewDNSProviderConfig(config)
+			require.NoError(t, err)
+
+			p.client = newMockClient().WithRegisterAccount(egTestAccount)
+
+			// Put
+			mux.HandleFunc("POST /example.com", func(w http.ResponseWriter, r *http.Request) {
+				w.WriteHeader(test.StatusCode)
+			})
+
+			acc, err := p.register(context.Background(), egDomain, egFQDN)
 			if test.ExpectedError != nil {
 				assert.Equal(t, test.ExpectedError, err)
 			} else {
 				require.NoError(t, err)
+				assert.Equal(t, egTestAccount, acc)
 			}
 		})
 	}
diff --git a/providers/dns/acmedns/mock_test.go b/providers/dns/acmedns/mock_test.go
index 0d188ecc..629f25a0 100644
--- a/providers/dns/acmedns/mock_test.go
+++ b/providers/dns/acmedns/mock_test.go
@@ -22,122 +22,139 @@ var egTestAccount = goacmedns.Account{
 	Password:   "trustno1",
 }
 
-// mockClient is a mock implementing the acmeDNSClient interface that always
-// returns a fixed goacmedns.Account from calls to Register.
 type mockClient struct {
-	mockAccount goacmedns.Account
-}
+	records map[goacmedns.Account]string
 
-// UpdateTXTRecord does nothing.
-func (c mockClient) UpdateTXTRecord(_ context.Context, _ goacmedns.Account, _ string) error {
-	return nil
-}
+	updateTXTRecordCalled bool
+	updateTXTRecord       func(ctx context.Context, acct goacmedns.Account, value string) error
 
-// RegisterAccount returns c.mockAccount and no errors.
-func (c mockClient) RegisterAccount(_ context.Context, _ []string) (goacmedns.Account, error) {
-	return c.mockAccount, nil
+	registerAccountCalled bool
+	registerAccount       func(ctx context.Context, allowFrom []string) (goacmedns.Account, error)
 }
 
-// mockUpdateClient is a mock implementing the acmeDNSClient interface that
-// tracks the calls to UpdateTXTRecord in the records map.
-type mockUpdateClient struct {
-	mockClient
-	records map[goacmedns.Account]string
+func newMockClient() *mockClient {
+	return &mockClient{
+		records: make(map[goacmedns.Account]string),
+		updateTXTRecord: func(_ context.Context, _ goacmedns.Account, _ string) error {
+			return nil
+		},
+		registerAccount: func(_ context.Context, _ []string) (goacmedns.Account, error) {
+			return goacmedns.Account{}, nil
+		},
+	}
 }
 
-// UpdateTXTRecord saves a record value to c.records for the given acct.
-func (c mockUpdateClient) UpdateTXTRecord(_ context.Context, acct goacmedns.Account, value string) error {
+func (c *mockClient) UpdateTXTRecord(ctx context.Context, acct goacmedns.Account, value string) error {
+	c.updateTXTRecordCalled = true
 	c.records[acct] = value
-	return nil
+
+	return c.updateTXTRecord(ctx, acct, value)
 }
 
-// errorUpdateClient is a mock implementing the acmeDNSClient interface that always
-// returns errors from errorUpdateClient.
-type errorUpdateClient struct {
-	mockClient
+func (c *mockClient) RegisterAccount(ctx context.Context, allowFrom []string) (goacmedns.Account, error) {
+	c.registerAccountCalled = true
+	return c.registerAccount(ctx, allowFrom)
 }
 
-// UpdateTXTRecord always returns an error.
-func (c errorUpdateClient) UpdateTXTRecord(_ context.Context, _ goacmedns.Account, _ string) error {
-	return errorClientErr
+func (c *mockClient) WithUpdateTXTRecordError(err error) *mockClient {
+	c.updateTXTRecord = func(_ context.Context, _ goacmedns.Account, _ string) error {
+		return err
+	}
+
+	return c
 }
 
-// errorRegisterClient is a mock implementing the acmeDNSClient interface that always
-// returns errors from RegisterAccount.
-type errorRegisterClient struct {
-	mockClient
+func (c *mockClient) WithRegisterAccount(acct goacmedns.Account) *mockClient {
+	c.registerAccount = func(_ context.Context, _ []string) (goacmedns.Account, error) {
+		return acct, nil
+	}
+
+	return c
 }
 
-// RegisterAccount always returns an error.
-func (c errorRegisterClient) RegisterAccount(_ context.Context, _ []string) (goacmedns.Account, error) {
-	return goacmedns.Account{}, errorClientErr
+func (c *mockClient) WithRegisterAccountError(err error) *mockClient {
+	c.registerAccount = func(_ context.Context, _ []string) (goacmedns.Account, error) {
+		return goacmedns.Account{}, err
+	}
+
+	return c
 }
 
-// mockStorage is a mock implementing the goacmedns.Storage interface that
-// returns static account data and ignores Save.
 type mockStorage struct {
 	accounts map[string]goacmedns.Account
-}
-
-// Save does nothing.
-func (m mockStorage) Save(_ context.Context) error {
-	return nil
-}
+	fetchAll func(ctx context.Context) (map[string]goacmedns.Account, error)
+	fetch    func(ctx context.Context, domain string) (goacmedns.Account, error)
+	put      func(ctx context.Context, domain string, acct goacmedns.Account) error
+	save     func(ctx context.Context) error
+}
+
+func newMockStorage() *mockStorage {
+	m := &mockStorage{
+		accounts: make(map[string]goacmedns.Account),
+		put: func(_ context.Context, _ string, _ goacmedns.Account) error {
+			return nil
+		},
+		save: func(_ context.Context) error {
+			return nil
+		},
+	}
 
-// Put stores an account for the given domain in m.accounts.
-func (m mockStorage) Put(_ context.Context, domain string, acct goacmedns.Account) error {
-	m.accounts[domain] = acct
-	return nil
-}
+	m.fetchAll = func(ctx context.Context) (map[string]goacmedns.Account, error) {
+		return m.accounts, nil
+	}
 
-// Fetch retrieves an account for the given domain from m.accounts or returns
-// goacmedns.ErrDomainNotFound.
-func (m mockStorage) Fetch(_ context.Context, domain string) (goacmedns.Account, error) {
-	if acct, ok := m.accounts[domain]; ok {
-		return acct, nil
+	m.fetch = func(_ context.Context, domain string) (goacmedns.Account, error) {
+		if acct, ok := m.accounts[domain]; ok {
+			return acct, nil
+		}
+		return goacmedns.Account{}, storage.ErrDomainNotFound
 	}
-	return goacmedns.Account{}, storage.ErrDomainNotFound
+
+	return m
 }
 
-// FetchAll returns all of m.accounts.
-func (m mockStorage) FetchAll(_ context.Context) (map[string]goacmedns.Account, error) {
-	return m.accounts, nil
+func (m *mockStorage) FetchAll(ctx context.Context) (map[string]goacmedns.Account, error) {
+	return m.fetchAll(ctx)
 }
 
-// errorPutStorage is a mock implementing the goacmedns.Storage interface that
-// always returns errors from Put.
-type errorPutStorage struct {
-	mockStorage
+func (m *mockStorage) Fetch(ctx context.Context, domain string) (goacmedns.Account, error) {
+	return m.fetch(ctx, domain)
 }
 
-// Put always errors.
-func (e errorPutStorage) Put(_ context.Context, _ string, _ goacmedns.Account) error {
-	return errorStorageErr
+func (m *mockStorage) Put(ctx context.Context, domain string, account goacmedns.Account) error {
+	return m.put(ctx, domain, account)
 }
 
-// errorSaveStorage is a mock implementing the goacmedns.Storage interface that
-// always returns errors from Save.
-type errorSaveStorage struct {
-	mockStorage
+func (m *mockStorage) Save(ctx context.Context) error {
+	return m.save(ctx)
 }
 
-// Save always errors.
-func (e errorSaveStorage) Save(_ context.Context) error {
-	return errorStorageErr
+func (m *mockStorage) WithAccount(domain string, acct goacmedns.Account) *mockStorage {
+	m.accounts[domain] = acct
+
+	return m
 }
 
-// errorFetchStorage is a mock implementing the goacmedns.Storage interface that
-// always returns errors from Fetch.
-type errorFetchStorage struct {
-	mockStorage
+func (m *mockStorage) WithFetchError(err error) *mockStorage {
+	m.fetch = func(_ context.Context, _ string) (goacmedns.Account, error) {
+		return goacmedns.Account{}, err
+	}
+
+	return m
 }
 
-// Fetch always errors.
-func (e errorFetchStorage) Fetch(_ context.Context, _ string) (goacmedns.Account, error) {
-	return goacmedns.Account{}, errorStorageErr
+func (m *mockStorage) WithPutError(err error) *mockStorage {
+	m.put = func(_ context.Context, _ string, _ goacmedns.Account) error {
+		return err
+	}
+
+	return m
 }
 
-// FetchAll is a nop for errorFetchStorage.
-func (e errorFetchStorage) FetchAll(_ context.Context) (map[string]goacmedns.Account, error) {
-	return nil, nil
+func (m *mockStorage) WithSaveError(err error) *mockStorage {
+	m.save = func(ctx context.Context) error {
+		return err
+	}
+
+	return m
 }