From 7829b80efe5552154acbcff6612ddae4bfc21052 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 20:36:10 +0100 Subject: [PATCH] fix(api): detect duplicate registrations by SQLSTATE, not error text 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 --- internal/api/handler_registrations.go | 35 ++++- .../api/handler_registrations_submit_test.go | 131 ++++++++++++++++++ .../config/store_postgres_registrations.go | 50 +++++-- ...postgres_registrations_integration_test.go | 70 ++++++++++ .../store_postgres_registrations_test.go | 96 +++++++++++++ 5 files changed, 368 insertions(+), 14 deletions(-) create mode 100644 internal/api/handler_registrations_submit_test.go create mode 100644 internal/config/store_postgres_registrations_integration_test.go create mode 100644 internal/config/store_postgres_registrations_test.go diff --git a/internal/api/handler_registrations.go b/internal/api/handler_registrations.go index eea4a26ea..1f6fa841f 100644 --- a/internal/api/handler_registrations.go +++ b/internal/api/handler_registrations.go @@ -102,11 +102,8 @@ func (h *Handler) submitRegistration(ctx context.Context, req *events.LambdaFunc } reg.RegCredentialPayload = encrypted - if err := h.config.CreateAccountRegistration(ctx, reg); err != nil { - if strings.Contains(err.Error(), "duplicate") { - return nil, NewClientError(409, "a pending registration already exists for this account") - } - return nil, fmt.Errorf("registrations: %w", err) + if err := h.insertRegistration(ctx, reg); err != nil { + return nil, err } // Notify admins (synchronous, errors logged but not propagated). @@ -127,11 +124,37 @@ func (h *Handler) submitRegistration(ctx context.Context, req *events.LambdaFunc } return map[string]string{ - "reference_token": token, + "reference_token": reg.ReferenceToken, "status": "pending", }, nil } +// maxReferenceTokenAttempts bounds how often insertRegistration regenerates a +// colliding reference token before giving up. +const maxReferenceTokenAttempts = 3 + +// insertRegistration stores reg, mapping a duplicate pending registration to +// 409 and regenerating reg.ReferenceToken on a token collision. Any other +// store error is returned wrapped, so it surfaces as a generic 500. +func (h *Handler) insertRegistration(ctx context.Context, reg *config.AccountRegistration) error { + for attempt := 1; ; attempt++ { + err := h.config.CreateAccountRegistration(ctx, reg) + switch { + case err == nil: + return nil + case errors.Is(err, config.ErrDuplicatePendingRegistration): + return NewClientError(409, "a pending registration already exists for this account") + case !errors.Is(err, config.ErrReferenceTokenCollision) || attempt >= maxReferenceTokenAttempts: + return fmt.Errorf("registrations: %w", err) + } + token, tokErr := generateReferenceToken() + if tokErr != nil { + return fmt.Errorf("registrations: generate token: %w", tokErr) + } + reg.ReferenceToken = token + } +} + // getRegistrationStatus handles GET /api/register/:token. func (h *Handler) getRegistrationStatus(ctx context.Context, token string) (any, error) { reg, err := h.config.GetAccountRegistrationByToken(ctx, token) diff --git a/internal/api/handler_registrations_submit_test.go b/internal/api/handler_registrations_submit_test.go new file mode 100644 index 000000000..019056b90 --- /dev/null +++ b/internal/api/handler_registrations_submit_test.go @@ -0,0 +1,131 @@ +package api + +// Tests for how submitRegistration maps store errors (issue #137): the 409 +// "already registered" answer comes only from the typed duplicate sentinel, +// never from the word "duplicate" in an error message. + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/jackc/pgx/v5/pgconn" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/email" +) + +// countingRegistrationNotifier counts admin "registration received" emails. +type countingRegistrationNotifier struct { + stubEmailNotifier + received int +} + +func (n *countingRegistrationNotifier) SendRegistrationReceivedNotification(_ context.Context, _ email.RegistrationNotificationData) error { + n.received++ + return nil +} + +const submitRegistrationBody = `{"provider":"aws","external_id":"123456789012",` + + `"account_name":"Prod","contact_email":"owner@example.com"}` + +func submitRegistrationRequest() *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{Body: submitRegistrationBody} +} + +func TestSubmitRegistration_DuplicatePending_Returns409WithoutEmail(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + store.On("CreateAccountRegistration", ctx, mock.Anything). + Return(fmt.Errorf("%w: %w", config.ErrDuplicatePendingRegistration, + &pgconn.PgError{Code: "23505", ConstraintName: "idx_account_registrations_pending_unique"})).Once() + notifier := &countingRegistrationNotifier{} + h := &Handler{config: store, emailNotifier: notifier} + + _, err := h.submitRegistration(ctx, submitRegistrationRequest()) + + ce, ok := IsClientError(err) + require.True(t, ok, "duplicate must be a client error, got %v", err) + assert.Equal(t, 409, ce.code) + assert.Equal(t, "a pending registration already exists for this account", ce.message) + assert.Zero(t, notifier.received, "a duplicate must not email admins") + store.AssertNumberOfCalls(t, "CreateAccountRegistration", 1) +} + +// Errors that are not the duplicate sentinel must surface as a generic 500, +// even when their text contains "duplicate", and must not leak the DB error. +func TestSubmitRegistration_NonDuplicateErrors_Return500(t *testing.T) { + cases := map[string]error{ + "text mentions duplicate": errors.New( + `duplicate: duplicate key value violates unique constraint "idx_account_registrations_pending_unique"`), + "unique violation on another constraint": fmt.Errorf("failed to create account registration: %w", + &pgconn.PgError{Code: "23505", ConstraintName: "account_registrations_pkey", + Message: "duplicate key value violates unique constraint"}), + "other database error": fmt.Errorf("failed to create account registration: %w", + &pgconn.PgError{Code: "53300", Message: "too many connections"}), + } + for name, storeErr := range cases { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + store.On("CreateAccountRegistration", ctx, mock.Anything).Return(storeErr).Once() + notifier := &countingRegistrationNotifier{} + h := &Handler{config: store, emailNotifier: notifier} + + _, err := h.submitRegistration(ctx, submitRegistrationRequest()) + + require.Error(t, err) + _, isClient := IsClientError(err) + assert.False(t, isClient, "must not be reported as a client (409) error") + status, body := h.handleRequestError(err) + assert.Equal(t, 500, status) + assert.Equal(t, map[string]string{"error": "Internal server error"}, body) + assert.Zero(t, notifier.received, "a duplicate must not email admins") + store.AssertNumberOfCalls(t, "CreateAccountRegistration", 1) + }) + } +} + +// A reference-token collision is retryable: the handler regenerates the token +// and re-inserts instead of telling the submitter the account is registered. +func TestSubmitRegistration_TokenCollision_RetriesWithNewToken(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + var tokens []string + collision := fmt.Errorf("%w: %w", config.ErrReferenceTokenCollision, + &pgconn.PgError{Code: "23505", ConstraintName: "account_registrations_reference_token_key"}) + record := func(args mock.Arguments) { + tokens = append(tokens, args.Get(1).(*config.AccountRegistration).ReferenceToken) + } + store.On("CreateAccountRegistration", ctx, mock.Anything).Run(record).Return(collision).Once() + store.On("CreateAccountRegistration", ctx, mock.Anything).Run(record).Return(nil).Once() + h := &Handler{config: store} + + resp, err := h.submitRegistration(ctx, submitRegistrationRequest()) + + require.NoError(t, err) + require.Len(t, tokens, 2) + assert.NotEqual(t, tokens[0], tokens[1], "retry must use a fresh token") + assert.Equal(t, map[string]string{"reference_token": tokens[1], "status": "pending"}, resp) +} + +func TestSubmitRegistration_TokenCollision_GivesUpAs500(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + collision := fmt.Errorf("%w: %w", config.ErrReferenceTokenCollision, + &pgconn.PgError{Code: "23505", ConstraintName: "account_registrations_reference_token_key"}) + store.On("CreateAccountRegistration", ctx, mock.Anything).Return(collision) + h := &Handler{config: store} + + _, err := h.submitRegistration(ctx, submitRegistrationRequest()) + + require.ErrorIs(t, err, config.ErrReferenceTokenCollision) + _, isClient := IsClientError(err) + assert.False(t, isClient) + store.AssertNumberOfCalls(t, "CreateAccountRegistration", maxReferenceTokenAttempts) +} diff --git a/internal/config/store_postgres_registrations.go b/internal/config/store_postgres_registrations.go index d181c2a78..0cfc22505 100644 --- a/internal/config/store_postgres_registrations.go +++ b/internal/config/store_postgres_registrations.go @@ -9,15 +9,53 @@ import ( "time" "github.com/google/uuid" + "github.com/jackc/pgx/v5/pgconn" ) // ErrRegistrationConflict is returned when a registration status transition fails // because another request already changed the status (concurrent modification). var ErrRegistrationConflict = errors.New("registration status conflict: already processed") -// CreateAccountRegistration inserts a new registration request. Returns an error -// wrapping "duplicate" when the partial unique index rejects a second pending -// registration for the same provider+external_id. +// ErrDuplicatePendingRegistration is returned by CreateAccountRegistration when +// a pending registration already exists for the same provider+external_id. +var ErrDuplicatePendingRegistration = errors.New("a pending registration already exists for this account") + +// ErrReferenceTokenCollision is returned by CreateAccountRegistration when the +// generated reference_token is already taken. The caller should regenerate the +// token and retry; it says nothing about whether the account is registered. +var ErrReferenceTokenCollision = errors.New("registration reference token collision") + +// Unique constraints on account_registrations (migration 000017). Matched by +// name so a new unique constraint cannot silently fold into an existing branch. +const ( + pgUniqueViolation = "23505" + registrationPendingUniqueIndex = "idx_account_registrations_pending_unique" + registrationReferenceTokenConstraint = "account_registrations_reference_token_key" +) + +// classifyRegistrationInsertError maps a unique violation on a known +// account_registrations constraint to its sentinel, using the driver's SQLSTATE +// and constraint name rather than the message text (which varies with +// lc_messages and driver versions). The driver error stays wrapped. Every +// other error, including unique violations on other constraints, is returned +// as a generic insert failure. +func classifyRegistrationInsertError(err error) error { + var pgErr *pgconn.PgError + if errors.As(err, &pgErr) && pgErr.Code == pgUniqueViolation { + switch pgErr.ConstraintName { + case registrationPendingUniqueIndex: + return fmt.Errorf("%w: %w", ErrDuplicatePendingRegistration, err) + case registrationReferenceTokenConstraint: + return fmt.Errorf("%w: %w", ErrReferenceTokenCollision, err) + } + } + return fmt.Errorf("failed to create account registration: %w", err) +} + +// CreateAccountRegistration inserts a new registration request. It returns an +// error wrapping ErrDuplicatePendingRegistration when the partial unique index +// rejects a second pending registration for the same provider+external_id, and +// one wrapping ErrReferenceTokenCollision when reg.ReferenceToken is taken. func (s *PostgresStore) CreateAccountRegistration(ctx context.Context, reg *AccountRegistration) error { if reg.ID == "" { reg.ID = uuid.New().String() @@ -61,11 +99,7 @@ func (s *PostgresStore) CreateAccountRegistration(ctx context.Context, reg *Acco reg.CreatedAt, reg.UpdatedAt, ) if err != nil { - if strings.Contains(err.Error(), "idx_account_registrations_pending_unique") || - strings.Contains(err.Error(), "duplicate key") { - return fmt.Errorf("duplicate: a pending registration already exists for this account") - } - return fmt.Errorf("failed to create account registration: %w", err) + return classifyRegistrationInsertError(err) } return nil } diff --git a/internal/config/store_postgres_registrations_integration_test.go b/internal/config/store_postgres_registrations_integration_test.go new file mode 100644 index 000000000..ca4bb9811 --- /dev/null +++ b/internal/config/store_postgres_registrations_integration_test.go @@ -0,0 +1,70 @@ +//go:build integration +// +build integration + +package config + +// Real-DB test for registration uniqueness (issue #137). The constraint names +// the store matches on come from migration 000017, so they are checked against +// the errors Postgres actually raises rather than hand-built ones. + +import ( + "context" + "testing" + + "github.com/jackc/pgx/v5/pgconn" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" +) + +func newPendingRegistration(token, externalID string) *AccountRegistration { + return &AccountRegistration{ + ReferenceToken: token, Status: "pending", Provider: "aws", ExternalID: externalID, + AccountName: "Prod", ContactEmail: "owner@example.com", + } +} + +func TestCreateAccountRegistration_UniquenessPostgres(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequirePostgresContainer(ctx, t) + t.Cleanup(func() { assert.NoError(t, container.Cleanup(context.Background())) }) + require.NoError(t, migrations.RunMigrations(ctx, container.DB.Pool(), getTestMigrationsPath(), "", "")) + store := NewPostgresStore(container.DB) + + first := newPendingRegistration("tok-137-a", "111111111111") + require.NoError(t, store.CreateAccountRegistration(ctx, first)) + + // Same account submitted twice while the first is pending. + err := store.CreateAccountRegistration(ctx, newPendingRegistration("tok-137-b", "111111111111")) + require.ErrorIs(t, err, ErrDuplicatePendingRegistration) + assert.NotErrorIs(t, err, ErrReferenceTokenCollision) + var pgErr *pgconn.PgError + require.ErrorAs(t, err, &pgErr) + assert.Equal(t, pgUniqueViolation, pgErr.Code) + assert.Equal(t, registrationPendingUniqueIndex, pgErr.ConstraintName) + + // A different account reusing an existing token is a token collision, + // not a duplicate registration. + err = store.CreateAccountRegistration(ctx, newPendingRegistration("tok-137-a", "222222222222")) + require.ErrorIs(t, err, ErrReferenceTokenCollision) + assert.NotErrorIs(t, err, ErrDuplicatePendingRegistration) + require.ErrorAs(t, err, &pgErr) + assert.Equal(t, registrationReferenceTokenConstraint, pgErr.ConstraintName) + + // A unique violation on any other constraint (here the primary key) is + // neither sentinel. + dupID := newPendingRegistration("tok-137-c", "333333333333") + dupID.ID = first.ID + err = store.CreateAccountRegistration(ctx, dupID) + require.Error(t, err) + assert.NotErrorIs(t, err, ErrDuplicatePendingRegistration) + assert.NotErrorIs(t, err, ErrReferenceTokenCollision) + + // Once the first registration leaves pending, the account may register again. + _, err = container.DB.Pool().Exec(ctx, + `UPDATE account_registrations SET status = 'rejected' WHERE id = $1`, first.ID) + require.NoError(t, err) + require.NoError(t, store.CreateAccountRegistration(ctx, newPendingRegistration("tok-137-d", "111111111111"))) +} diff --git a/internal/config/store_postgres_registrations_test.go b/internal/config/store_postgres_registrations_test.go new file mode 100644 index 000000000..b0aa5981a --- /dev/null +++ b/internal/config/store_postgres_registrations_test.go @@ -0,0 +1,96 @@ +package config + +// Unit tests for how CreateAccountRegistration classifies insert errors +// (issue #137): unique violations are recognized by SQLSTATE and constraint +// name, never by the words in the error message. + +import ( + "context" + "errors" + "testing" + + "github.com/jackc/pgx/v5/pgconn" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func execRegistrationInsertError(t *testing.T, dbErr error) error { + t.Helper() + mock := newMock(t) + defer mock.Close() + mock.ExpectExec("INSERT INTO account_registrations"). + WithArgs(anyArgsCfg(24)...). + WillReturnError(dbErr) + + err := storeWith(mock).CreateAccountRegistration(context.Background(), &AccountRegistration{ + ReferenceToken: "tok", Status: "pending", Provider: "aws", ExternalID: "123456789012", + AccountName: "acct", ContactEmail: "a@example.com", + }) + require.NoError(t, mock.ExpectationsWereMet()) + require.Error(t, err) + return err +} + +func TestCreateAccountRegistration_PendingUniqueViolation_IsDuplicate(t *testing.T) { + pgErr := &pgconn.PgError{Code: "23505", ConstraintName: "idx_account_registrations_pending_unique", + Message: "duplicate key value violates unique constraint"} + err := execRegistrationInsertError(t, pgErr) + + assert.ErrorIs(t, err, ErrDuplicatePendingRegistration) + assert.NotErrorIs(t, err, ErrReferenceTokenCollision) + var got *pgconn.PgError + require.ErrorAs(t, err, &got, "driver error must stay reachable for callers") + assert.Same(t, pgErr, got) +} + +// A locale-translated (lc_messages) server message carries no English words; +// the SQLSTATE and constraint name still identify it. +func TestCreateAccountRegistration_TranslatedMessage_IsDuplicate(t *testing.T) { + err := execRegistrationInsertError(t, &pgconn.PgError{Code: "23505", + ConstraintName: "idx_account_registrations_pending_unique", + Message: "llave duplicada viola restricción de unicidad"}) + assert.ErrorIs(t, err, ErrDuplicatePendingRegistration) +} + +func TestCreateAccountRegistration_ReferenceTokenViolation_IsCollisionNotDuplicate(t *testing.T) { + err := execRegistrationInsertError(t, &pgconn.PgError{Code: "23505", + ConstraintName: "account_registrations_reference_token_key", + Message: `duplicate key value violates unique constraint "account_registrations_reference_token_key"`}) + + assert.ErrorIs(t, err, ErrReferenceTokenCollision) + assert.NotErrorIs(t, err, ErrDuplicatePendingRegistration) +} + +func TestCreateAccountRegistration_NonDuplicateErrors_AreNotDuplicate(t *testing.T) { + cases := map[string]error{ + "unique violation on another constraint": &pgconn.PgError{Code: "23505", + ConstraintName: "account_registrations_pkey", + Message: `duplicate key value violates unique constraint "account_registrations_pkey"`}, + "other SQLSTATE naming the pending index": &pgconn.PgError{Code: "23514", + ConstraintName: "idx_account_registrations_pending_unique", + Message: "duplicate key check failed"}, + "plain error whose text says duplicate key": errors.New( + `duplicate key value violates unique constraint "idx_account_registrations_pending_unique"`), + } + for name, dbErr := range cases { + t.Run(name, func(t *testing.T) { + err := execRegistrationInsertError(t, dbErr) + assert.NotErrorIs(t, err, ErrDuplicatePendingRegistration) + assert.NotErrorIs(t, err, ErrReferenceTokenCollision) + assert.ErrorIs(t, err, dbErr, "original error must stay wrapped") + }) + } +} + +func TestCreateAccountRegistration_Success(t *testing.T) { + mock := newMock(t) + defer mock.Close() + mock.ExpectExec("INSERT INTO account_registrations"). + WithArgs(anyArgsCfg(24)...). + WillReturnResult(pgconn.NewCommandTag("INSERT 0 1")) + + reg := &AccountRegistration{ReferenceToken: "tok", Status: "pending", Provider: "aws"} + require.NoError(t, storeWith(mock).CreateAccountRegistration(context.Background(), reg)) + assert.NotEmpty(t, reg.ID) + require.NoError(t, mock.ExpectationsWereMet()) +}