Skip to content

Commit b56fe1f

Browse files
IAmJSDclaude
andauthored
fix(api): detect duplicate registrations by SQLSTATE, not error text (#1019)
CreateAccountRegistration classified insert failures by substring-matching "duplicate key" / the index name in the driver error and returned a hand-built "duplicate: ..." string, which the register handler then matched with strings.Contains(err.Error(), "duplicate"). A reference_token collision was misreported as an existing registration (409), any error whose text said "duplicate" became a 409, and a translated or reformatted server message would turn a routine duplicate into a 500. The store now uses errors.As to find the *pgconn.PgError and switches on SQLSTATE 23505 plus the constraint name, returning ErrDuplicatePendingRegistration for idx_account_registrations_pending_unique and ErrReferenceTokenCollision for account_registrations_reference_token_key, both wrapping the driver error. Every other error stays a generic insert failure. The handler maps the duplicate sentinel to the same 409 and message as before via errors.Is, regenerates the token and retries (bounded) on a token collision, and lets everything else surface as a generic 500. Closes #137 Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 70a6457 commit b56fe1f

5 files changed

Lines changed: 368 additions & 14 deletions

‎internal/api/handler_registrations.go‎

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -102,11 +102,8 @@ func (h *Handler) submitRegistration(ctx context.Context, req *events.LambdaFunc
102102
}
103103
reg.RegCredentialPayload = encrypted
104104

105-
if err := h.config.CreateAccountRegistration(ctx, reg); err != nil {
106-
if strings.Contains(err.Error(), "duplicate") {
107-
return nil, NewClientError(409, "a pending registration already exists for this account")
108-
}
109-
return nil, fmt.Errorf("registrations: %w", err)
105+
if err := h.insertRegistration(ctx, reg); err != nil {
106+
return nil, err
110107
}
111108

112109
// Notify admins (synchronous, errors logged but not propagated).
@@ -127,11 +124,37 @@ func (h *Handler) submitRegistration(ctx context.Context, req *events.LambdaFunc
127124
}
128125

129126
return map[string]string{
130-
"reference_token": token,
127+
"reference_token": reg.ReferenceToken,
131128
"status": "pending",
132129
}, nil
133130
}
134131

132+
// maxReferenceTokenAttempts bounds how often insertRegistration regenerates a
133+
// colliding reference token before giving up.
134+
const maxReferenceTokenAttempts = 3
135+
136+
// insertRegistration stores reg, mapping a duplicate pending registration to
137+
// 409 and regenerating reg.ReferenceToken on a token collision. Any other
138+
// store error is returned wrapped, so it surfaces as a generic 500.
139+
func (h *Handler) insertRegistration(ctx context.Context, reg *config.AccountRegistration) error {
140+
for attempt := 1; ; attempt++ {
141+
err := h.config.CreateAccountRegistration(ctx, reg)
142+
switch {
143+
case err == nil:
144+
return nil
145+
case errors.Is(err, config.ErrDuplicatePendingRegistration):
146+
return NewClientError(409, "a pending registration already exists for this account")
147+
case !errors.Is(err, config.ErrReferenceTokenCollision) || attempt >= maxReferenceTokenAttempts:
148+
return fmt.Errorf("registrations: %w", err)
149+
}
150+
token, tokErr := generateReferenceToken()
151+
if tokErr != nil {
152+
return fmt.Errorf("registrations: generate token: %w", tokErr)
153+
}
154+
reg.ReferenceToken = token
155+
}
156+
}
157+
135158
// getRegistrationStatus handles GET /api/register/:token.
136159
func (h *Handler) getRegistrationStatus(ctx context.Context, token string) (any, error) {
137160
reg, err := h.config.GetAccountRegistrationByToken(ctx, token)
Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
package api
2+
3+
// Tests for how submitRegistration maps store errors (issue #137): the 409
4+
// "already registered" answer comes only from the typed duplicate sentinel,
5+
// never from the word "duplicate" in an error message.
6+
7+
import (
8+
"context"
9+
"errors"
10+
"fmt"
11+
"testing"
12+
13+
"github.com/aws/aws-lambda-go/events"
14+
"github.com/jackc/pgx/v5/pgconn"
15+
"github.com/stretchr/testify/assert"
16+
"github.com/stretchr/testify/mock"
17+
"github.com/stretchr/testify/require"
18+
19+
"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
20+
"github.com/LeanerCloud/cloud-commitments-platform/internal/email"
21+
)
22+
23+
// countingRegistrationNotifier counts admin "registration received" emails.
24+
type countingRegistrationNotifier struct {
25+
stubEmailNotifier
26+
received int
27+
}
28+
29+
func (n *countingRegistrationNotifier) SendRegistrationReceivedNotification(_ context.Context, _ email.RegistrationNotificationData) error {
30+
n.received++
31+
return nil
32+
}
33+
34+
const submitRegistrationBody = `{"provider":"aws","external_id":"123456789012",` +
35+
`"account_name":"Prod","contact_email":"owner@example.com"}`
36+
37+
func submitRegistrationRequest() *events.LambdaFunctionURLRequest {
38+
return &events.LambdaFunctionURLRequest{Body: submitRegistrationBody}
39+
}
40+
41+
func TestSubmitRegistration_DuplicatePending_Returns409WithoutEmail(t *testing.T) {
42+
ctx := context.Background()
43+
store := new(MockConfigStore)
44+
store.On("CreateAccountRegistration", ctx, mock.Anything).
45+
Return(fmt.Errorf("%w: %w", config.ErrDuplicatePendingRegistration,
46+
&pgconn.PgError{Code: "23505", ConstraintName: "idx_account_registrations_pending_unique"})).Once()
47+
notifier := &countingRegistrationNotifier{}
48+
h := &Handler{config: store, emailNotifier: notifier}
49+
50+
_, err := h.submitRegistration(ctx, submitRegistrationRequest())
51+
52+
ce, ok := IsClientError(err)
53+
require.True(t, ok, "duplicate must be a client error, got %v", err)
54+
assert.Equal(t, 409, ce.code)
55+
assert.Equal(t, "a pending registration already exists for this account", ce.message)
56+
assert.Zero(t, notifier.received, "a duplicate must not email admins")
57+
store.AssertNumberOfCalls(t, "CreateAccountRegistration", 1)
58+
}
59+
60+
// Errors that are not the duplicate sentinel must surface as a generic 500,
61+
// even when their text contains "duplicate", and must not leak the DB error.
62+
func TestSubmitRegistration_NonDuplicateErrors_Return500(t *testing.T) {
63+
cases := map[string]error{
64+
"text mentions duplicate": errors.New(
65+
`duplicate: duplicate key value violates unique constraint "idx_account_registrations_pending_unique"`),
66+
"unique violation on another constraint": fmt.Errorf("failed to create account registration: %w",
67+
&pgconn.PgError{Code: "23505", ConstraintName: "account_registrations_pkey",
68+
Message: "duplicate key value violates unique constraint"}),
69+
"other database error": fmt.Errorf("failed to create account registration: %w",
70+
&pgconn.PgError{Code: "53300", Message: "too many connections"}),
71+
}
72+
for name, storeErr := range cases {
73+
t.Run(name, func(t *testing.T) {
74+
ctx := context.Background()
75+
store := new(MockConfigStore)
76+
store.On("CreateAccountRegistration", ctx, mock.Anything).Return(storeErr).Once()
77+
notifier := &countingRegistrationNotifier{}
78+
h := &Handler{config: store, emailNotifier: notifier}
79+
80+
_, err := h.submitRegistration(ctx, submitRegistrationRequest())
81+
82+
require.Error(t, err)
83+
_, isClient := IsClientError(err)
84+
assert.False(t, isClient, "must not be reported as a client (409) error")
85+
status, body := h.handleRequestError(err)
86+
assert.Equal(t, 500, status)
87+
assert.Equal(t, map[string]string{"error": "Internal server error"}, body)
88+
assert.Zero(t, notifier.received, "a duplicate must not email admins")
89+
store.AssertNumberOfCalls(t, "CreateAccountRegistration", 1)
90+
})
91+
}
92+
}
93+
94+
// A reference-token collision is retryable: the handler regenerates the token
95+
// and re-inserts instead of telling the submitter the account is registered.
96+
func TestSubmitRegistration_TokenCollision_RetriesWithNewToken(t *testing.T) {
97+
ctx := context.Background()
98+
store := new(MockConfigStore)
99+
var tokens []string
100+
collision := fmt.Errorf("%w: %w", config.ErrReferenceTokenCollision,
101+
&pgconn.PgError{Code: "23505", ConstraintName: "account_registrations_reference_token_key"})
102+
record := func(args mock.Arguments) {
103+
tokens = append(tokens, args.Get(1).(*config.AccountRegistration).ReferenceToken)
104+
}
105+
store.On("CreateAccountRegistration", ctx, mock.Anything).Run(record).Return(collision).Once()
106+
store.On("CreateAccountRegistration", ctx, mock.Anything).Run(record).Return(nil).Once()
107+
h := &Handler{config: store}
108+
109+
resp, err := h.submitRegistration(ctx, submitRegistrationRequest())
110+
111+
require.NoError(t, err)
112+
require.Len(t, tokens, 2)
113+
assert.NotEqual(t, tokens[0], tokens[1], "retry must use a fresh token")
114+
assert.Equal(t, map[string]string{"reference_token": tokens[1], "status": "pending"}, resp)
115+
}
116+
117+
func TestSubmitRegistration_TokenCollision_GivesUpAs500(t *testing.T) {
118+
ctx := context.Background()
119+
store := new(MockConfigStore)
120+
collision := fmt.Errorf("%w: %w", config.ErrReferenceTokenCollision,
121+
&pgconn.PgError{Code: "23505", ConstraintName: "account_registrations_reference_token_key"})
122+
store.On("CreateAccountRegistration", ctx, mock.Anything).Return(collision)
123+
h := &Handler{config: store}
124+
125+
_, err := h.submitRegistration(ctx, submitRegistrationRequest())
126+
127+
require.ErrorIs(t, err, config.ErrReferenceTokenCollision)
128+
_, isClient := IsClientError(err)
129+
assert.False(t, isClient)
130+
store.AssertNumberOfCalls(t, "CreateAccountRegistration", maxReferenceTokenAttempts)
131+
}

‎internal/config/store_postgres_registrations.go‎

Lines changed: 42 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,53 @@ import (
99
"time"
1010

1111
"github.com/google/uuid"
12+
"github.com/jackc/pgx/v5/pgconn"
1213
)
1314

1415
// ErrRegistrationConflict is returned when a registration status transition fails
1516
// because another request already changed the status (concurrent modification).
1617
var ErrRegistrationConflict = errors.New("registration status conflict: already processed")
1718

18-
// CreateAccountRegistration inserts a new registration request. Returns an error
19-
// wrapping "duplicate" when the partial unique index rejects a second pending
20-
// registration for the same provider+external_id.
19+
// ErrDuplicatePendingRegistration is returned by CreateAccountRegistration when
20+
// a pending registration already exists for the same provider+external_id.
21+
var ErrDuplicatePendingRegistration = errors.New("a pending registration already exists for this account")
22+
23+
// ErrReferenceTokenCollision is returned by CreateAccountRegistration when the
24+
// generated reference_token is already taken. The caller should regenerate the
25+
// token and retry; it says nothing about whether the account is registered.
26+
var ErrReferenceTokenCollision = errors.New("registration reference token collision")
27+
28+
// Unique constraints on account_registrations (migration 000017). Matched by
29+
// name so a new unique constraint cannot silently fold into an existing branch.
30+
const (
31+
pgUniqueViolation = "23505"
32+
registrationPendingUniqueIndex = "idx_account_registrations_pending_unique"
33+
registrationReferenceTokenConstraint = "account_registrations_reference_token_key"
34+
)
35+
36+
// classifyRegistrationInsertError maps a unique violation on a known
37+
// account_registrations constraint to its sentinel, using the driver's SQLSTATE
38+
// and constraint name rather than the message text (which varies with
39+
// lc_messages and driver versions). The driver error stays wrapped. Every
40+
// other error, including unique violations on other constraints, is returned
41+
// as a generic insert failure.
42+
func classifyRegistrationInsertError(err error) error {
43+
var pgErr *pgconn.PgError
44+
if errors.As(err, &pgErr) && pgErr.Code == pgUniqueViolation {
45+
switch pgErr.ConstraintName {
46+
case registrationPendingUniqueIndex:
47+
return fmt.Errorf("%w: %w", ErrDuplicatePendingRegistration, err)
48+
case registrationReferenceTokenConstraint:
49+
return fmt.Errorf("%w: %w", ErrReferenceTokenCollision, err)
50+
}
51+
}
52+
return fmt.Errorf("failed to create account registration: %w", err)
53+
}
54+
55+
// CreateAccountRegistration inserts a new registration request. It returns an
56+
// error wrapping ErrDuplicatePendingRegistration when the partial unique index
57+
// rejects a second pending registration for the same provider+external_id, and
58+
// one wrapping ErrReferenceTokenCollision when reg.ReferenceToken is taken.
2159
func (s *PostgresStore) CreateAccountRegistration(ctx context.Context, reg *AccountRegistration) error {
2260
if reg.ID == "" {
2361
reg.ID = uuid.New().String()
@@ -61,11 +99,7 @@ func (s *PostgresStore) CreateAccountRegistration(ctx context.Context, reg *Acco
6199
reg.CreatedAt, reg.UpdatedAt,
62100
)
63101
if err != nil {
64-
if strings.Contains(err.Error(), "idx_account_registrations_pending_unique") ||
65-
strings.Contains(err.Error(), "duplicate key") {
66-
return fmt.Errorf("duplicate: a pending registration already exists for this account")
67-
}
68-
return fmt.Errorf("failed to create account registration: %w", err)
102+
return classifyRegistrationInsertError(err)
69103
}
70104
return nil
71105
}
Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
//go:build integration
2+
// +build integration
3+
4+
package config
5+
6+
// Real-DB test for registration uniqueness (issue #137). The constraint names
7+
// the store matches on come from migration 000017, so they are checked against
8+
// the errors Postgres actually raises rather than hand-built ones.
9+
10+
import (
11+
"context"
12+
"testing"
13+
14+
"github.com/jackc/pgx/v5/pgconn"
15+
"github.com/stretchr/testify/assert"
16+
"github.com/stretchr/testify/require"
17+
18+
"github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations"
19+
"github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers"
20+
)
21+
22+
func newPendingRegistration(token, externalID string) *AccountRegistration {
23+
return &AccountRegistration{
24+
ReferenceToken: token, Status: "pending", Provider: "aws", ExternalID: externalID,
25+
AccountName: "Prod", ContactEmail: "owner@example.com",
26+
}
27+
}
28+
29+
func TestCreateAccountRegistration_UniquenessPostgres(t *testing.T) {
30+
ctx := context.Background()
31+
container := testhelpers.RequirePostgresContainer(ctx, t)
32+
t.Cleanup(func() { assert.NoError(t, container.Cleanup(context.Background())) })
33+
require.NoError(t, migrations.RunMigrations(ctx, container.DB.Pool(), getTestMigrationsPath(), "", ""))
34+
store := NewPostgresStore(container.DB)
35+
36+
first := newPendingRegistration("tok-137-a", "111111111111")
37+
require.NoError(t, store.CreateAccountRegistration(ctx, first))
38+
39+
// Same account submitted twice while the first is pending.
40+
err := store.CreateAccountRegistration(ctx, newPendingRegistration("tok-137-b", "111111111111"))
41+
require.ErrorIs(t, err, ErrDuplicatePendingRegistration)
42+
assert.NotErrorIs(t, err, ErrReferenceTokenCollision)
43+
var pgErr *pgconn.PgError
44+
require.ErrorAs(t, err, &pgErr)
45+
assert.Equal(t, pgUniqueViolation, pgErr.Code)
46+
assert.Equal(t, registrationPendingUniqueIndex, pgErr.ConstraintName)
47+
48+
// A different account reusing an existing token is a token collision,
49+
// not a duplicate registration.
50+
err = store.CreateAccountRegistration(ctx, newPendingRegistration("tok-137-a", "222222222222"))
51+
require.ErrorIs(t, err, ErrReferenceTokenCollision)
52+
assert.NotErrorIs(t, err, ErrDuplicatePendingRegistration)
53+
require.ErrorAs(t, err, &pgErr)
54+
assert.Equal(t, registrationReferenceTokenConstraint, pgErr.ConstraintName)
55+
56+
// A unique violation on any other constraint (here the primary key) is
57+
// neither sentinel.
58+
dupID := newPendingRegistration("tok-137-c", "333333333333")
59+
dupID.ID = first.ID
60+
err = store.CreateAccountRegistration(ctx, dupID)
61+
require.Error(t, err)
62+
assert.NotErrorIs(t, err, ErrDuplicatePendingRegistration)
63+
assert.NotErrorIs(t, err, ErrReferenceTokenCollision)
64+
65+
// Once the first registration leaves pending, the account may register again.
66+
_, err = container.DB.Pool().Exec(ctx,
67+
`UPDATE account_registrations SET status = 'rejected' WHERE id = $1`, first.ID)
68+
require.NoError(t, err)
69+
require.NoError(t, store.CreateAccountRegistration(ctx, newPendingRegistration("tok-137-d", "111111111111")))
70+
}

0 commit comments

Comments
 (0)