Repository: favonia/cloudflare-ddns (go)

Issue:
Add hints for common cloudflare errors
Since 1.14.0, we no longer do remote token verification, and sadly there are no official documentation on the error codes returned by Cloudflare, but we can still "guess" the fixes based on the server's current behavior. For example:

1. 10000 probably means the API token is invalid or insufficient permissions are given.
2. (more to add to this issue)

Pull request description:
docs: hint about permissions for DNS records
Close #897.

Patch:
diff --git a/internal/api/cloudflare_records.go b/internal/api/cloudflare_records.go
index fba43e3..6fc5108 100644
--- a/internal/api/cloudflare_records.go
+++ b/internal/api/cloudflare_records.go
@@ -2,6 +2,7 @@ package api
 
 import (
 	"context"
+	"errors"
 	"net/netip"
 	"slices"
 
@@ -13,6 +14,16 @@ import (
 	"github.com/favonia/cloudflare-ddns/internal/pp"
 )
 
+func hintRecordPermission(ppfmt pp.PP, err error) {
+	var authentication *cloudflare.AuthenticationError
+	var authorization *cloudflare.AuthorizationError
+	if errors.As(err, &authentication) || errors.As(err, &authorization) {
+		ppfmt.Hintf(pp.HintRecordPermission,
+			"Double check your API token. "+
+				`Make sure you granted the "Edit" permission of "Zone - DNS"`)
+	}
+}
+
 // ListZones returns a list of zone IDs with the zone name.
 func (h CloudflareHandle) ListZones(ctx context.Context, ppfmt pp.PP, name string) ([]ID, bool) {
 	// WithZoneFilters does not work with the empty zone name,
@@ -28,6 +39,7 @@ func (h CloudflareHandle) ListZones(ctx context.Context, ppfmt pp.PP, name strin
 	res, err := h.cf.ListZonesContext(ctx, cloudflare.WithZoneFilters(name, "", ""))
 	if err != nil {
 		ppfmt.Noticef(pp.EmojiError, "Failed to check the existence of a zone named %q: %v", name, err)
+		hintRecordPermission(ppfmt, err)
 		return nil, false
 	}
 
@@ -119,6 +131,7 @@ func (h CloudflareHandle) ListRecords(ctx context.Context, ppfmt pp.PP,
 		ppfmt.Noticef(pp.EmojiError,
 			"Failed to retrieve %s records of %q: %v",
 			ipNet.RecordType(), domain.Describe(), err)
+		hintRecordPermission(ppfmt, err)
 		return nil, false, false
 	}
 
@@ -153,6 +166,7 @@ func (h CloudflareHandle) DeleteRecord(ctx context.Context, ppfmt pp.PP,
 	if err := h.cf.DeleteDNSRecord(ctx, cloudflare.ZoneIdentifier(string(zone)), string(id)); err != nil {
 		ppfmt.Noticef(pp.EmojiError, "Failed to delete a stale %s record of %q (ID: %s): %v",
 			ipNet.RecordType(), domain.Describe(), id, err)
+		hintRecordPermission(ppfmt, err)
 
 		if !keepCacheWhenFails {
 			h.cache.listRecords[ipNet].Delete(domain.DNSNameASCII())
@@ -188,6 +202,7 @@ func (h CloudflareHandle) UpdateRecord(ctx context.Context, ppfmt pp.PP,
 	if _, err := h.cf.UpdateDNSRecord(ctx, cloudflare.ZoneIdentifier(string(zone)), params); err != nil {
 		ppfmt.Noticef(pp.EmojiError, "Failed to update a stale %s record of %q (ID: %s): %v",
 			ipNet.RecordType(), domain.Describe(), id, err)
+		hintRecordPermission(ppfmt, err)
 
 		h.cache.listRecords[ipNet].Delete(domain.DNSNameASCII())
 
@@ -228,6 +243,7 @@ func (h CloudflareHandle) CreateRecord(ctx context.Context, ppfmt pp.PP,
 	if err != nil {
 		ppfmt.Noticef(pp.EmojiError, "Failed to add a new %s record of %q: %v",
 			ipNet.RecordType(), domain.Describe(), err)
+		hintRecordPermission(ppfmt, err)
 
 		h.cache.listRecords[ipNet].Delete(domain.DNSNameASCII())
 
diff --git a/internal/api/cloudflare_waf.go b/internal/api/cloudflare_waf.go
index f52b9b9..e069283 100644
--- a/internal/api/cloudflare_waf.go
+++ b/internal/api/cloudflare_waf.go
@@ -28,9 +28,11 @@ var WAFListMaxBitLen = map[ipnet.Type]int{ //nolint:gochecknoglobals
 
 func hintWAFListPermission(ppfmt pp.PP, err error) {
 	var authentication *cloudflare.AuthenticationError
-	if errors.As(err, &authentication) {
+	var authorization *cloudflare.AuthorizationError
+	if errors.As(err, &authentication) || errors.As(err, &authorization) {
 		ppfmt.Hintf(pp.HintWAFListPermission,
-			`Make sure you granted the "Edit" permission of "Account - Account Filter Lists"`)
+			"Double check your API token and account ID. "+
+				`Make sure you granted the "Edit" permission of "Account - Account Filter Lists"`)
 	}
 }
 
diff --git a/internal/pp/hint.go b/internal/pp/hint.go
index 179c1c8..9d31fb9 100644
--- a/internal/pp/hint.go
+++ b/internal/pp/hint.go
@@ -10,6 +10,7 @@ const (
 	HintIP6DetectionFails
 	HintDetectionTimeouts
 	HintUpdateTimeouts
+	HintRecordPermission
 	HintWAFListPermission
 	Hint1111Blockage
 )


Test patch:
diff --git a/internal/api/cloudflare_records_test.go b/internal/api/cloudflare_records_test.go
index b61aa12..04c4533 100644
--- a/internal/api/cloudflare_records_test.go
+++ b/internal/api/cloudflare_records_test.go
@@ -93,13 +93,7 @@ func TestListZonesTwo(t *testing.T) {
 		ok           bool
 		output       []api.ID
 	}{
-		"root": {
-			nil,
-			0,
-			"",
-			true,
-			[]api.ID{},
-		},
+		"root": {nil, 0, "", true, []api.ID{}},
 		"two": {
 			map[string][]string{"test.org": {"active", "active"}},
 			1,
@@ -349,6 +343,8 @@ func newListRecordsHandler(t *testing.T, mux *http.ServeMux,
 		func(w http.ResponseWriter, r *http.Request) {
 			if !checkRequestLimit(t, &requestLimit) || !checkToken(t, r) {
 				w.WriteHeader(http.StatusUnauthorized)
+				_, err := w.Write([]byte(`{"success":false,"errors":[{"code":9109,"message":"Invalid access token"}],"messages":[],"result":null}`)) //nolint:lll
+				assert.NoError(t, err)
 				return
 			}
 
@@ -370,170 +366,158 @@ func newListRecordsHandler(t *testing.T, mux *http.ServeMux,
 	return httpHandler{requestLimit: &requestLimit}
 }
 
-//nolint:dupl
 func TestListRecords(t *testing.T) {
 	t.Parallel()
-	mockCtrl := gomock.NewController(t)
-	mockPP := mocks.NewMockPP(mockCtrl)
-
-	mux, h, ok := newHandle(t, mockPP)
-	require.True(t, ok)
-
-	zh := newZonesHandler(t, mux, map[string][]string{"test.org": {"active"}})
-	zh.setRequestLimit(2)
-
-	lrh := newListRecordsHandler(t, mux, ipnet.IP6, "sub.test.org",
-		[]formattedRecord{{"record1", "::1"}, {"record2", "::2"}})
-	lrh.setRequestLimit(1)
-
-	expected := []api.Record{{"record1", mustIP("::1")}, {"record2", mustIP("::2")}}
-
-	rs, cached, ok := h.ListRecords(context.Background(), mockPP, ipnet.IP6, domain.FQDN("sub.test.org"))
-	require.True(t, ok)
-	require.False(t, cached)
-	require.Equal(t, expected, rs)
-	require.True(t, lrh.isExhausted())
-
-	// testing the caching
-	mockPP = mocks.NewMockPP(mockCtrl)
-	rs, cached, ok = h.ListRecords(context.Background(), mockPP, ipnet.IP6, domain.FQDN("sub.test.org"))
-	require.True(t, ok)
-	require.True(t, cached)
-	require.Equal(t, expected, rs)
-}
-
-func TestListRecordsInvalidIPAddress(t *testing.T) {
-	t.Parallel()
-	mockCtrl := gomock.NewController(t)
-	mockPP := mocks.NewMockPP(mockCtrl)
-
-	mux, h, ok := newHandle(t, mockPP)
-	require.True(t, ok)
 
-	zh := newZonesHandler(t, mux, map[string][]string{"test.org": {"active"}})
-	zh.setRequestLimit(2)
-
-	lrh := newListRecordsHandler(t, mux, ipnet.IP6, "sub.test.org",
-		[]formattedRecord{{"record1", "::1"}, {"record2", "not an ip"}})
-	lrh.setRequestLimit(1)
-
-	mockPP.EXPECT().Noticef(
-		pp.EmojiImpossible,
-		"Failed to parse the IP address in an %s record of %q (ID: %s): %v",
-		"AAAA",
-		"sub.test.org",
-		"record2",
-		gomock.Any(),
-	)
-	rs, cached, ok := h.ListRecords(context.Background(), mockPP, ipnet.IP6, domain.FQDN("sub.test.org"))
-	require.False(t, ok)
-	require.False(t, cached)
-	require.Zero(t, rs)
-	require.True(t, lrh.isExhausted())
-
-	// testing the (no) caching
-	mockPP = mocks.NewMockPP(mockCtrl)
-	mockPP.EXPECT().Noticef(
-		pp.EmojiError,
-		"Failed to retrieve %s records of %q: %v",
-		"AAAA",
-		"sub.test.org",
-		gomock.Any(),
-	)
-	rs, cached, ok = h.ListRecords(context.Background(), mockPP, ipnet.IP6, domain.FQDN("sub.test.org"))
-	require.False(t, ok)
-	require.False(t, cached)
-	require.Zero(t, rs)
-	require.True(t, lrh.isExhausted())
-}
-
-//nolint:dupl
-func TestListRecordsWildcard(t *testing.T) {
-	t.Parallel()
-	mockCtrl := gomock.NewController(t)
-	mockPP := mocks.NewMockPP(mockCtrl)
-
-	mux, h, ok := newHandle(t, mockPP)
-	require.True(t, ok)
-
-	zh := newZonesHandler(t, mux, map[string][]string{"test.org": {"active"}})
-	zh.setRequestLimit(2)
-
-	lrh := newListRecordsHandler(t, mux, ipnet.IP6, "*.test.org",
-		[]formattedRecord{{"record1", "::1"}, {"record2", "::2"}})
-	lrh.setRequestLimit(1)
-
-	expected := []api.Record{{"record1", mustIP("::1")}, {"record2", mustIP("::2")}}
-
-	rs, cached, ok := h.ListRecords(context.Background(), mockPP, ipnet.IP6, domain.Wildcard("test.org"))
-	require.True(t, ok)
-	require.False(t, cached)
-	require.Equal(t, expected, rs)
-	require.True(t, lrh.isExhausted())
-
-	// testing the caching
-	mockPP = mocks.NewMockPP(mockCtrl)
-	rs, cached, ok = h.ListRecords(context.Background(), mockPP, ipnet.IP6, domain.Wildcard("test.org"))
-	require.True(t, ok)
-	require.True(t, cached)
-	require.Equal(t, expected, rs)
-}
-
-func TestListRecordsInvalidDomain(t *testing.T) {
-	t.Parallel()
-	mockCtrl := gomock.NewController(t)
-	mockPP := mocks.NewMockPP(mockCtrl)
-
-	mux, h, ok := newHandle(t, mockPP)
-	require.True(t, ok)
-
-	zh := newZonesHandler(t, mux, map[string][]string{"test.org": {"active"}})
-	zh.setRequestLimit(2)
+	for name, tc := range map[string]struct {
+		zones                 map[string][]string
+		zoneRequestLimit      int
+		recordDomain          string
+		records               []formattedRecord
+		listRequestLimit      int
+		input                 domain.Domain
+		expected              []api.Record
+		ok                    bool
+		prepareMocks          func(*mocks.MockPP)
+		cached                bool
+		prepareMocksForCached func(*mocks.MockPP)
+	}{
+		"success": {
+			map[string][]string{"test.org": {"active"}},
+			2,
+			"sub.test.org",
+			[]formattedRecord{{"record1", "::1"}, {"record2", "::2"}},
+			1,
+			domain.FQDN("sub.test.org"),
+			[]api.Record{{"record1", mustIP("::1")}, {"record2", mustIP("::2")}},
+			true,
+			nil, true, nil,
+		},
+		"success/wildcard": {
+			map[string][]string{"test.org": {"active"}},
+			1,
+			"*.test.org",
+			[]formattedRecord{{"record1", "::1"}, {"record2", "::2"}},
+			1,
+			domain.Wildcard("test.org"),
+			[]api.Record{{"record1", mustIP("::1")}, {"record2", mustIP("::2")}},
+			true,
+			nil, true, nil,
+		},
+		"list-fail": {
+			map[string][]string{"test.org": {"active"}},
+			2,
+			"sub.test.org", nil, 0,
+			domain.FQDN("sub.test.org"),
+			nil,
+			false,
+			func(ppfmt *mocks.MockPP) {
+				ppfmt.EXPECT().Noticef(
+					pp.EmojiError,
+					"Failed to retrieve %s records of %q: %v", "AAAA", "sub.test.org", gomock.Any(),
+				)
+				ppfmt.EXPECT().Hintf(pp.HintRecordPermission,
+					"Double check your API token. "+
+						`Make sure you granted the "Edit" permission of "Zone - DNS"`)
+			},
+			false,
+			func(ppfmt *mocks.MockPP) {
+				ppfmt.EXPECT().Noticef(
+					pp.EmojiError,
+					"Failed to retrieve %s records of %q: %v", "AAAA", "sub.test.org", gomock.Any(),
+				)
+				ppfmt.EXPECT().Hintf(pp.HintRecordPermission,
+					"Double check your API token. "+
+						`Make sure you granted the "Edit" permission of "Zone - DNS"`)
+			},
+		},
+		"no-zone": {
+			nil, 0,
+			"sub.test.org", nil, 0,
+			domain.FQDN("sub.test.org"),
+			nil,
+			false,
+			func(ppfmt *mocks.MockPP) {
+				ppfmt.EXPECT().Noticef(
+					pp.EmojiError,
+					"Failed to check the existence of a zone named %q: %v",
+					"sub.test.org", gomock.Any(),
+				)
+			},
+			false,
+			func(ppfmt *mocks.MockPP) {
+				ppfmt.EXPECT().Noticef(
+					pp.EmojiError,
+					"Failed to check the existence of a zone named %q: %v",
+					"sub.test.org", gomock.Any(),
+				)
+			},
+		},
+		"invalid-ip": {
+			map[string][]string{"test.org": {"active"}},
+			2,
+			"sub.test.org",
+			[]formattedRecord{{"record1", "::1"}, {"record2", "not an ip"}},
+			1,
+			domain.FQDN("sub.test.org"),
+			nil,
+			false,
+			func(ppfmt *mocks.MockPP) {
+				ppfmt.EXPECT().Noticef(
+					pp.EmojiImpossible,
+					"Failed to parse the IP address in an %s record of %q (ID: %s): %v",
+					"AAAA", "sub.test.org", "record2", gomock.Any(),
+				)
+			},
+			false,
+			func(ppfmt *mocks.MockPP) {
+				ppfmt.EXPECT().Noticef(
+					pp.EmojiError,
+					"Failed to retrieve %s records of %q: %v",
+					"AAAA", "sub.test.org", gomock.Any(),
+				)
+				ppfmt.EXPECT().Hintf(pp.HintRecordPermission,
+					"Double check your API token. "+
+						`Make sure you granted the "Edit" permission of "Zone - DNS"`)
+			},
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
 
-	mockPP.EXPECT().Noticef(pp.EmojiError, "Failed to retrieve %s records of %q: %v", "A", "sub.test.org", gomock.Any())
-	rs, cached, ok := h.ListRecords(context.Background(), mockPP, ipnet.IP4, domain.FQDN("sub.test.org"))
-	require.False(t, ok)
-	require.False(t, cached)
-	require.Zero(t, rs)
+			mockCtrl := gomock.NewController(t)
+			mockPP := mocks.NewMockPP(mockCtrl)
+			if tc.prepareMocks != nil {
+				tc.prepareMocks(mockPP)
+			}
 
-	mockPP = mocks.NewMockPP(mockCtrl)
-	mockPP.EXPECT().Noticef(pp.EmojiError, "Failed to retrieve %s records of %q: %v", "AAAA", "sub.test.org", gomock.Any()) //nolint:lll
-	rs, cached, ok = h.ListRecords(context.Background(), mockPP, ipnet.IP6, domain.FQDN("sub.test.org"))
-	require.False(t, ok)
-	require.False(t, cached)
-	require.Zero(t, rs)
-}
+			mux, h, ok := newHandle(t, mockPP)
+			require.True(t, ok)
 
-func TestListRecordsInvalidZone(t *testing.T) {
-	t.Parallel()
-	mockCtrl := gomock.NewController(t)
-	mockPP := mocks.NewMockPP(mockCtrl)
+			zh := newZonesHandler(t, mux, tc.zones)
+			zh.setRequestLimit(tc.zoneRequestLimit)
 
-	_, h, ok := newHandle(t, mockPP)
-	require.True(t, ok)
+			lrh := newListRecordsHandler(t, mux, ipnet.IP6, tc.recordDomain, tc.records)
+			lrh.setRequestLimit(tc.listRequestLimit)
 
-	mockPP.EXPECT().Noticef(
-		pp.EmojiError,
-		"Failed to check the existence of a zone named %q: %v",
-		"sub.test.org",
-		gomock.Any(),
-	)
-	rs, cached, ok := h.ListRecords(context.Background(), mockPP, ipnet.IP4, domain.FQDN("sub.test.org"))
-	require.False(t, ok)
-	require.False(t, cached)
-	require.Zero(t, rs)
+			rs, cached, ok := h.ListRecords(context.Background(), mockPP, ipnet.IP6, tc.input)
+			require.Equal(t, tc.ok, ok)
+			require.False(t, cached)
+			require.Equal(t, tc.expected, rs)
+			require.True(t, zh.isExhausted())
+			require.True(t, lrh.isExhausted())
 
-	mockPP = mocks.NewMockPP(mockCtrl)
-	mockPP.EXPECT().Noticef(
-		pp.EmojiError,
-		"Failed to check the existence of a zone named %q: %v",
-		"sub.test.org",
-		gomock.Any(),
-	)
-	rs, cached, ok = h.ListRecords(context.Background(), mockPP, ipnet.IP4, domain.FQDN("sub.test.org"))
-	require.False(t, ok)
-	require.False(t, cached)
-	require.Zero(t, rs)
+			// testing the caching
+			mockPP = mocks.NewMockPP(mockCtrl)
+			if tc.prepareMocksForCached != nil {
+				tc.prepareMocksForCached(mockPP)
+			}
+			rs, cached, ok = h.ListRecords(context.Background(), mockPP, ipnet.IP6, tc.input)
+			require.Equal(t, tc.ok, ok)
+			require.Equal(t, tc.cached, cached)
+			require.Equal(t, tc.expected, rs)
+		})
+	}
 }
 
 func envelopDNSRecordResponse(record cloudflare.DNSRecord) cloudflare.DNSRecordResponse {
diff --git a/internal/api/cloudflare_waf_test.go b/internal/api/cloudflare_waf_test.go
index ec03101..0fddeb2 100644
--- a/internal/api/cloudflare_waf_test.go
+++ b/internal/api/cloudflare_waf_test.go
@@ -189,7 +189,8 @@ func TestListWAFListsHint(t *testing.T) {
 	gomock.InOrder(
 		mockPP.EXPECT().Noticef(pp.EmojiError, "Failed to list existing lists: %v", gomock.Any()),
 		mockPP.EXPECT().Hintf(pp.HintWAFListPermission,
-			`Make sure you granted the "Edit" permission of "Account - Account Filter Lists"`),
+			"Double check your API token and account ID. "+
+				`Make sure you granted the "Edit" permission of "Account - Account Filter Lists"`),
 	)
 	lists, ok := h.(api.CloudflareHandle).ListWAFLists(context.Background(), mockPP, mockAccountID)
 	require.False(t, ok)