diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index f80f3554e..d5e003783 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1965,29 +1965,29 @@ func checkRevokableStatus(execution *config.PurchaseExecution) error { } } -// validateRevokeToken checks that the execution carries a non-empty -// ApprovalToken that matches the supplied token (constant-time comparison), -// then that the token has not expired and that the revocation window has not -// closed (config.RevocationWindow after CompletedAt). Pre-migration rows -// without an ApprovalTokenExpiresAt pass the expiry check (legacy backward -// compat). +// validateRevokeToken checks the revocation token with the same status +// mapping as the approve and cancel links (tokenActionError, issue #587): a +// missing or non-matching token is 403 and a matching but expired token is +// 410. Pre-migration rows without an ApprovalTokenExpiresAt pass the expiry +// check (legacy backward compat). Last, the 24-hour revocation window must +// still be open (config.RevocationWindow after CompletedAt). // -// The token is compared first, as on the approve path (ValidateApprovalToken): -// a wrong token must get the same "invalid" answer whatever the row's expiry or -// window, or the expiry/window message tells a token guesser that the link -// exists and has lapsed (issue #1190). +// The token is compared first, as on the approve path: it shares +// purchase.ValidateApprovalToken, which matches the token (in constant time, +// against the SHA-256 digest stored at rest, issue #103) before it reads the +// expiry. A wrong token must get the same "invalid" answer whatever the row's +// expiry or window, or the expiry/window message tells a token guesser that +// the link exists and has lapsed (issue #1190). // Extracted from revokeViaEmailToken to keep that function under the cyclomatic limit. func validateRevokeToken(execution *config.PurchaseExecution, token string) error { - // execution.ApprovalToken is the SHA-256 hex digest stored at rest - // (issue #103), not the raw secret; config.ApprovalTokenMatches hashes - // the supplied token and compares digests in constant time, and is false - // for an empty stored or supplied token. - if !config.ApprovalTokenMatches(execution.ApprovalToken, token) { + switch err := purchase.ValidateApprovalToken(execution, token); { + case errors.Is(err, purchase.ErrInvalidApprovalToken): return NewClientError(403, "invalid revocation token") - } - if execution.ApprovalTokenExpiresAt != nil && time.Now().After(*execution.ApprovalTokenExpiresAt) { - return NewClientError(409, fmt.Sprintf( + case errors.Is(err, purchase.ErrApprovalTokenExpired): + return NewClientError(410, fmt.Sprintf( "execution %s cannot be revoked: the revocation link has expired", execution.ExecutionID)) + case err != nil: + return err } // Enforce the 24-hour revocation window (issue #291 Finding #6). // Use CompletedAt as the reference timestamp; fall back to ExecutedAt diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 1c008ba7d..ef1294510 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -5937,8 +5937,9 @@ func TestHandler_revokePurchase_NotFound(t *testing.T) { } // TestHandler_revokePurchase_ExpiredToken verifies that a matching token whose -// ApprovalTokenExpiresAt deadline has passed is rejected with 409, so a stale -// completed execution cannot be marked revocation_requested via an old link. +// ApprovalTokenExpiresAt deadline has passed is rejected with 410 (409 before +// issue #587 aligned revoke with approve and cancel), so a stale completed +// execution cannot be marked revocation_requested via an old link. // Regression test for the revoke-token expiry CR finding (PR #889). func TestHandler_revokePurchase_ExpiredToken(t *testing.T) { ctx := context.Background() @@ -5985,7 +5986,7 @@ func TestHandler_revokePurchase_ExpiredToken(t *testing.T) { require.Error(t, err, "expired revocation token must be rejected") ce, ok := IsClientError(err) require.True(t, ok, "expected a client error") - assert.Equal(t, 409, ce.code) + assert.Equal(t, 410, ce.code) assert.Contains(t, ce.message, "expired") } diff --git a/internal/api/handler_purchases_token_errors_test.go b/internal/api/handler_purchases_token_errors_test.go index 7e169dbeb..c0850bf59 100644 --- a/internal/api/handler_purchases_token_errors_test.go +++ b/internal/api/handler_purchases_token_errors_test.go @@ -18,7 +18,8 @@ import ( // Email-link approve/cancel and the email revoke endpoint are driven through // HandleRequest with the real purchase.Manager so the real token and status -// errors reach the HTTP mapping (issue #564). +// errors reach the HTTP mapping (issue #564). All three share one mapping: +// invalid token 403, expired token 410, wrong status 409 (issue #587). const tokenErrExecID = "12345678-1234-1234-1234-123456789abc" const tokenErrRawToken = "raw-secret-token" @@ -86,6 +87,13 @@ func TestHandleRequest_TokenActions_TokenAndStatusErrorsAre4xx(t *testing.T) { {"cancel invalid token", "cancel", config.PurchaseExecution{Status: "pending", ApprovalToken: hash, ApprovalTokenExpiresAt: &future}, "wrong", 403}, {"cancel expired token", "cancel", config.PurchaseExecution{Status: "pending", ApprovalToken: hash, ApprovalTokenExpiresAt: &past}, tokenErrRawToken, 410}, {"cancel non-cancelable status", "cancel", config.PurchaseExecution{Status: "completed", ApprovalToken: hash, ApprovalTokenExpiresAt: &future}, tokenErrRawToken, 409}, + // Issue #587: revoke uses the same mapping as approve and cancel, and + // the token is matched before its expiry is read. + {"approve wrong token on an expired link", "approve", config.PurchaseExecution{Status: "pending", ApprovalToken: hash, ApprovalTokenExpiresAt: &past}, "wrong", 403}, + {"revoke invalid token", "revoke", config.PurchaseExecution{Status: "completed", ApprovalToken: hash, ApprovalTokenExpiresAt: &future}, "wrong", 403}, + {"revoke expired token", "revoke", config.PurchaseExecution{Status: "completed", ApprovalToken: hash, ApprovalTokenExpiresAt: &past}, tokenErrRawToken, 410}, + {"revoke wrong token on an expired link", "revoke", config.PurchaseExecution{Status: "completed", ApprovalToken: hash, ApprovalTokenExpiresAt: &past}, "wrong", 403}, + {"revoke non-revocable status", "revoke", config.PurchaseExecution{Status: "pending", ApprovalToken: hash, ApprovalTokenExpiresAt: &future}, tokenErrRawToken, 409}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -185,7 +193,7 @@ func TestHandleRequest_RevokeViaEmail_WrongTokenOnLapsedRowIsInvalid(t *testing. }{ {"wrong token on an expired link", config.PurchaseExecution{Status: "completed", ApprovalToken: hash, ApprovalTokenExpiresAt: &past}, "wrong", 403, "invalid revocation token"}, {"wrong token past the revocation window", config.PurchaseExecution{Status: "completed", ApprovalToken: hash, ApprovalTokenExpiresAt: &future, CompletedAt: &longAgo}, "wrong", 403, "invalid revocation token"}, - {"right token on an expired link", config.PurchaseExecution{Status: "completed", ApprovalToken: hash, ApprovalTokenExpiresAt: &past}, tokenErrRawToken, 409, "expired"}, + {"right token on an expired link", config.PurchaseExecution{Status: "completed", ApprovalToken: hash, ApprovalTokenExpiresAt: &past}, tokenErrRawToken, 410, "expired"}, {"right token past the revocation window", config.PurchaseExecution{Status: "completed", ApprovalToken: hash, ApprovalTokenExpiresAt: &future, CompletedAt: &longAgo}, tokenErrRawToken, 403, "revocation window has closed"}, } for _, tc := range cases { diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index ac0989fcf..69ff4161e 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -2449,10 +2449,10 @@ func (h *Handler) approveRIExchangeViaToken(ctx context.Context, req *events.Lam transitioned, err := h.config.TransitionRIExchangeStatus(ctx, id, "pending", "processing", resolveCreatorUserID(session)) if err != nil { - return nil, fmt.Errorf("failed to transition exchange status: %w", err) + return nil, riExchangeTransitionError(err, riExchangeApproveConflictMsg) } if transitioned == nil { - return nil, NewClientError(409, "exchange already processed, expired, or was canceled by a newer analysis run") + return nil, NewClientError(409, riExchangeApproveConflictMsg) } result, execErr := h.executeApprovedExchange(ctx, id, record) @@ -2512,10 +2512,10 @@ func (h *Handler) approveRIExchangeViaSession(ctx context.Context, req *events.L // Session-authed approval: stamp the session user as the actor. transitioned, err := h.config.TransitionRIExchangeStatus(ctx, id, "pending", "processing", resolveCreatorUserID(session)) if err != nil { - return nil, fmt.Errorf("failed to transition exchange status: %w", err) + return nil, riExchangeTransitionError(err, riExchangeApproveConflictMsg) } if transitioned == nil { - return nil, NewClientError(409, "exchange already processed, expired, or was canceled by a newer analysis run") + return nil, NewClientError(409, riExchangeApproveConflictMsg) } result, execErr := h.executeApprovedExchange(ctx, id, record) @@ -2730,19 +2730,62 @@ func (h *Handler) validateExchangeApproval(ctx context.Context, id, token string return nil, NewClientError(404, "exchange record not found") } - if record.ApprovalToken == "" { - return nil, NewClientError(403, "this exchange record does not support approval") + if err := checkExchangeToken(record, token, "approval"); err != nil { + return nil, err } + return record, nil +} + +const ( + riExchangeApproveConflictMsg = "exchange already processed or was canceled by a newer analysis run" + riExchangeRejectConflictMsg = "exchange already processed or was canceled" + riExchangeExpiredMsg = "exchange has expired" +) - // record.ApprovalToken is the SHA-256 hex digest stored at rest - // (issue #103); hash the supplied token and compare digests in - // constant time, matching config.ApprovalTokenMatches everywhere else - // in this codebase that validates one of these tokens. +// checkExchangeToken applies the emailed-token status mapping shared with the +// purchase approve, cancel and revoke links (issue #587): a record without a +// token, or a token that does not match, is 403, and a matching token on an +// exchange past its expires_at is 410. The match is checked first, so only +// the token holder learns that the exchange expired. kind is "approval" or +// "rejection" and only shapes the 403 text. +// +// record.ApprovalToken is the SHA-256 hex digest stored at rest (issue #103); +// config.ApprovalTokenMatches hashes the supplied token and compares digests +// in constant time, as everywhere else that validates one of these tokens. +func checkExchangeToken(record *config.RIExchangeRecord, token, kind string) error { + if record.ApprovalToken == "" { + return NewClientError(403, "this exchange record does not support "+kind) + } if !config.ApprovalTokenMatches(record.ApprovalToken, token) { - return nil, NewClientError(403, "invalid approval token") + return NewClientError(403, "invalid "+kind+" token") } + if record.ExpiresAt != nil && time.Now().After(*record.ExpiresAt) { + return NewClientError(410, riExchangeExpiredMsg) + } + return nil +} - return record, nil +// riExchangeTransitionError maps a lost pending -> X transition: the record +// expired (410) or another writer moved its status first (409), the same +// codes as an expired or already-handled purchase email link (issue #587). +// The expiry pre-check in checkExchangeToken cannot close the window alone: +// the record can expire, or a concurrent approve or reject can win, between +// that check and the transition. Anything else stays a server error. +// +// handleRequestError does not log ClientErrors, so the 409 and 410 branches +// log the store's error themselves: it names the exchange and its current +// status, which the response body does not. +func riExchangeTransitionError(err error, conflictMsg string) error { + switch { + case errors.Is(err, config.ErrRIExchangeExpired): + logging.Infof("ri-exchange transition lost to expiry: %v", err) + return NewClientError(410, riExchangeExpiredMsg) + case errors.Is(err, config.ErrRIExchangeNotInExpectedStatus): + logging.Infof("ri-exchange transition lost to a concurrent writer: %v", err) + return NewClientError(409, conflictMsg) + default: + return fmt.Errorf("failed to transition exchange status: %w", err) + } } // failExchange marks an exchange as failed, logging if the DB write also fails. @@ -2898,23 +2941,17 @@ func (h *Handler) rejectRIExchange(ctx context.Context, id, token string) (any, return nil, NewClientError(404, "exchange record not found") } - if record.ApprovalToken == "" { - return nil, NewClientError(403, "this exchange record does not support rejection") - } - - // See validateExchangeApproval: record.ApprovalToken is a SHA-256 hex - // digest (issue #103), not the raw secret. - if !config.ApprovalTokenMatches(record.ApprovalToken, token) { - return nil, NewClientError(403, "invalid rejection token") + if tokenErr := checkExchangeToken(record, token, "rejection"); tokenErr != nil { + return nil, tokenErr } // Token-based rejection: no session user, so transitioned_by = NULL. transitioned, err := h.config.TransitionRIExchangeStatus(ctx, id, "pending", config.StatusCanceled, nil) if err != nil { - return nil, fmt.Errorf("failed to transition exchange status: %w", err) + return nil, riExchangeTransitionError(err, riExchangeRejectConflictMsg) } if transitioned == nil { - return nil, NewClientError(409, "exchange already processed, expired, or was canceled") + return nil, NewClientError(409, riExchangeRejectConflictMsg) } return map[string]string{"status": config.StatusCanceled}, nil diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 60a542f9c..d80632770 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -1234,7 +1234,16 @@ paths: '409': description: > The execution status cannot be revoked (still pending or - notified, or not completed), or the revocation link has expired. + notified, or not completed), or its status changed concurrently. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + '410': + description: > + Token mode only, the revocation token matches but has expired. + The token is matched first, so a wrong token on an expired link + is 403, as on approve and cancel. content: application/json: schema: @@ -2515,8 +2524,19 @@ paths: $ref: '#/components/responses/NotFound' '409': description: > - The exchange is no longer pending (already processed, expired or - canceled by a newer analysis run). + The exchange is no longer pending (already processed, or canceled + by a newer analysis run), including when a concurrent approve or + reject changed it first. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + '410': + description: > + The exchange has expired (its expires_at has passed). Token mode + checks this once the token matches, so a wrong token on an + expired exchange is 403; both modes also get 410 when the + exchange expires before the atomic transition. content: application/json: schema: @@ -2569,7 +2589,17 @@ paths: '404': $ref: '#/components/responses/NotFound' '409': - description: The exchange is no longer pending (already processed, expired or canceled). + description: > + The exchange is no longer pending (already processed or canceled), + including when a concurrent approve or reject changed it first. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + '410': + description: > + The rejection token matches but the exchange has expired (its + expires_at has passed). A wrong token on an expired exchange is 403. content: application/json: schema: diff --git a/internal/api/ri_exchange_approve_reject_status_test.go b/internal/api/ri_exchange_approve_reject_status_test.go index 84519eea6..380b4d1ea 100644 --- a/internal/api/ri_exchange_approve_reject_status_test.go +++ b/internal/api/ri_exchange_approve_reject_status_test.go @@ -4,7 +4,9 @@ import ( "context" "encoding/json" "errors" + "fmt" "testing" + "time" "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" @@ -28,8 +30,36 @@ const ( riRecNoToken riRecMissing riRecAlreadyProcessed + // riRecExpired is pending but past its expires_at. Its transition is + // mocked to succeed, so a 410 proves the token check answered before + // any write (the real CAS would refuse too; see riRecTransitionExpired). + riRecExpired + // riRecTransitionExpired expires between the token check and the + // transition: the store reports ErrRIExchangeExpired, as Postgres does. + riRecTransitionExpired + // riRecTransitionConflict loses the transition to a concurrent approve or + // reject: the store reports ErrRIExchangeNotInExpectedStatus. + riRecTransitionConflict + // riRecTransitionFails hits a store error unrelated to the record state. + riRecTransitionFails ) +// riStatusTransition returns the mocked TransitionRIExchangeStatus result for rec. +func riStatusTransition(rec riStatusRecord) (*config.RIExchangeRecord, error) { + switch rec { + case riRecAlreadyProcessed: + return nil, nil + case riRecTransitionExpired: + return nil, fmt.Errorf("%w: %s", config.ErrRIExchangeExpired, riStatusID) + case riRecTransitionConflict: + return nil, fmt.Errorf("%w: expected status %q but current status is %q", config.ErrRIExchangeNotInExpectedStatus, "pending", "processing") + case riRecTransitionFails: + return nil, errors.New("connection reset") + default: + return &config.RIExchangeRecord{ID: riStatusID, Status: "processing"}, nil + } +} + // riStatusOpts tweaks the fixture for the less common cases. type riStatusOpts struct { fourEyes bool // global 4-eyes mode on; the admin session is the creator @@ -50,6 +80,10 @@ func riStatusHandler(rec riStatusRecord, o riStatusOpts) *Handler { if rec == riRecNoToken { record.ApprovalToken = "" } + if rec == riRecExpired { + past := time.Now().Add(-time.Hour) + record.ExpiresAt = &past + } if (o.fourEyes && !o.noCreator) || o.approveOwn { creator := "admin-uuid" if o.approveOwn { @@ -64,12 +98,9 @@ func riStatusHandler(rec riStatusRecord, o riStatusOpts) *Handler { } else { store.On("GetRIExchangeRecord", mock.Anything, riStatusID).Return(record, nil).Maybe() } - var transitioned *config.RIExchangeRecord - if rec != riRecAlreadyProcessed { - transitioned = &config.RIExchangeRecord{ID: riStatusID, Status: "processing"} - } + transitioned, transitionErr := riStatusTransition(rec) store.On("TransitionRIExchangeStatus", mock.Anything, riStatusID, "pending", mock.Anything, mock.Anything). - Return(transitioned, nil).Maybe() + Return(transitioned, transitionErr).Maybe() store.On("ReserveApprovedRIExchange", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return("500.000000", nil).Maybe() store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{RequireDifferentApprover: o.fourEyes}, nil).Maybe() store.On("FailRIExchange", mock.Anything, riStatusID, mock.Anything).Return(nil).Maybe() @@ -192,7 +223,17 @@ func TestHandleRequest_ApproveRIExchange_StatusCodes(t *testing.T) { {name: "404 missing record with token", rec: riRecMissing, sess: riSessNone, token: riStatusRawToken, want: 404, wantError: "exchange record not found"}, {name: "404 missing record with wrong token", rec: riRecMissing, sess: riSessNone, token: "wrong", want: 404, wantError: "exchange record not found"}, {name: "404 missing record with session", rec: riRecMissing, sess: riSessCSRF, want: 404, wantError: "exchange record not found"}, - {name: "409 token approve of an exchange no longer pending", rec: riRecAlreadyProcessed, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed, expired, or was canceled by a newer analysis run"}, + {name: "409 token approve of an exchange no longer pending", rec: riRecAlreadyProcessed, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed or was canceled by a newer analysis run"}, + // Issue #587: the same invalid/expired/wrong-status mapping as the + // purchase approve, cancel and revoke links (403/410/409). + {name: "410 token approve of an expired exchange", rec: riRecExpired, sess: riSessNone, token: riStatusRawToken, want: 410, wantError: "exchange has expired"}, + {name: "403 wrong token on an expired exchange does not reveal the expiry", rec: riRecExpired, sess: riSessNone, token: "wrong", want: 403, wantError: "invalid approval token"}, + {name: "410 expired exchange answers before the 4-eyes session check", rec: riRecExpired, sess: riSessNone, token: riStatusRawToken, opts: riStatusOpts{fourEyes: true, noCreator: true}, want: 410, wantError: "exchange has expired"}, + {name: "410 token approve when the exchange expires before the transition", rec: riRecTransitionExpired, sess: riSessNone, token: riStatusRawToken, want: 410, wantError: "exchange has expired"}, + {name: "409 token approve loses the transition to a concurrent writer", rec: riRecTransitionConflict, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed or was canceled by a newer analysis run"}, + {name: "500 token approve transition store failure", rec: riRecTransitionFails, sess: riSessNone, token: riStatusRawToken, want: 500}, + {name: "410 session approve when the exchange expires before the transition", rec: riRecTransitionExpired, sess: riSessCSRF, want: 410, wantError: "exchange has expired"}, + {name: "409 session approve loses the transition to a concurrent writer", rec: riRecTransitionConflict, sess: riSessCSRF, want: 409, wantError: "exchange already processed or was canceled by a newer analysis run"}, }) } @@ -212,6 +253,12 @@ func TestHandleRequest_RejectRIExchange_StatusCodes(t *testing.T) { {name: "403 record without approval token", rec: riRecNoToken, sess: riSessNone, token: riStatusRawToken, want: 403, wantError: "this exchange record does not support rejection"}, {name: "404 missing record with token", rec: riRecMissing, sess: riSessNone, token: riStatusRawToken, want: 404, wantError: "exchange record not found"}, {name: "404 missing record with wrong token", rec: riRecMissing, sess: riSessNone, token: "wrong", want: 404, wantError: "exchange record not found"}, - {name: "409 exchange no longer pending", rec: riRecAlreadyProcessed, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed, expired, or was canceled"}, + {name: "409 exchange no longer pending", rec: riRecAlreadyProcessed, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed or was canceled"}, + // Issue #587: 403 invalid, 410 expired, 409 wrong status. + {name: "410 expired exchange", rec: riRecExpired, sess: riSessNone, token: riStatusRawToken, want: 410, wantError: "exchange has expired"}, + {name: "403 wrong token on an expired exchange does not reveal the expiry", rec: riRecExpired, sess: riSessNone, token: "wrong", want: 403, wantError: "invalid rejection token"}, + {name: "410 exchange expires before the transition", rec: riRecTransitionExpired, sess: riSessNone, token: riStatusRawToken, want: 410, wantError: "exchange has expired"}, + {name: "409 loses the transition to a concurrent writer", rec: riRecTransitionConflict, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed or was canceled"}, + {name: "500 transition store failure", rec: riRecTransitionFails, sess: riSessNone, token: riStatusRawToken, want: 500}, }) } diff --git a/internal/config/errors.go b/internal/config/errors.go index 75982d6a4..cd60fa293 100644 --- a/internal/config/errors.go +++ b/internal/config/errors.go @@ -32,6 +32,18 @@ var ErrManagedPlanLiveExecutions = errors.New("managed plan still has active or // distinguish this legitimate race-loss from a hard DB error. var ErrExecutionNotInExpectedStatus = errors.New("execution not in expected status") +// ErrRIExchangeExpired is returned (wrapped) by TransitionRIExchangeStatus +// when the record exists but its expires_at has passed, so the approval or +// rejection window is closed. The HTTP layer answers it with 410, like an +// expired purchase approval token (issue #587). +var ErrRIExchangeExpired = errors.New("ri exchange has expired") + +// ErrRIExchangeNotInExpectedStatus is returned (wrapped) by +// TransitionRIExchangeStatus when the record exists and has not expired but +// its status is not the expected one: another writer, such as a concurrent +// approve or reject, transitioned it first. The HTTP layer answers it with 409. +var ErrRIExchangeNotInExpectedStatus = errors.New("ri exchange status transition failed") + // ErrApprovalTokenChanged is returned (wrapped) by // TransitionExecutionStatusTokenPinned when the execution is still in an // allowed status but its stored approval token no longer matches the one the diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index b56c63d25..9f6ce0bd4 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -516,6 +516,8 @@ type StoreInterface interface { GetRIExchangeHistoryForAccounts(ctx context.Context, since time.Time, limit int, accountIDs []string) ([]RIExchangeRecord, error) // TransitionRIExchangeStatus atomically transitions an RI exchange record status. // actor is the UUID of the user performing the transition (nil for system-initiated paths). + // A lost transition returns a wrapped ErrRIExchangeExpired when the record + // has expired, or ErrRIExchangeNotInExpectedStatus when its status moved on. TransitionRIExchangeStatus(ctx context.Context, id string, fromStatus string, toStatus string, actor *string) (*RIExchangeRecord, error) CompleteRIExchange(ctx context.Context, id string, exchangeID string) error // CompleteRIExchangeWithPayment marks an RI exchange as completed and diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 5628ff41a..058cd80fe 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -3478,7 +3478,8 @@ func (s *PostgresStore) GetRIExchangeHistoryForAccounts(ctx context.Context, sin // TransitionRIExchangeStatus atomically transitions an RI exchange record status. // Uses a single UPDATE...WHERE...RETURNING for atomicity, then diagnoses failure -// only if zero rows are returned. +// only if zero rows are returned (ErrRIExchangeExpired or +// ErrRIExchangeNotInExpectedStatus, wrapped, for an existing record). // actor is the UUID of the user performing the transition (nil for system-initiated paths). func (s *PostgresStore) TransitionRIExchangeStatus(ctx context.Context, id, fromStatus, toStatus string, actor *string) (*RIExchangeRecord, error) { query := ` @@ -3521,9 +3522,9 @@ func (s *PostgresStore) diagnoseTransitionFailure(ctx context.Context, id, fromS return fmt.Errorf("failed to diagnose transition failure: %w", err) } if expired { - return fmt.Errorf("ri exchange has expired") + return fmt.Errorf("%w: %s", ErrRIExchangeExpired, id) } - return fmt.Errorf("ri exchange status transition failed: expected status %q but current status is %q", fromStatus, currentStatus) + return fmt.Errorf("%w: expected status %q but current status is %q", ErrRIExchangeNotInExpectedStatus, fromStatus, currentStatus) } // CompleteRIExchange marks an RI exchange as completed. diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index dcbc936a0..6ca6e26aa 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -1486,6 +1486,8 @@ func TestPGXMock_TransitionRIExchangeStatus_WrongStatus(t *testing.T) { _, err := store.TransitionRIExchangeStatus(ctx, "ri-id", "pending", "processing", nil) require.Error(t, err) assert.Contains(t, err.Error(), "expected status") + assert.ErrorIs(t, err, ErrRIExchangeNotInExpectedStatus, "the API maps this to 409 (issue #587)") + assert.NotErrorIs(t, err, ErrRIExchangeExpired) } func TestPGXMock_TransitionRIExchangeStatus_RecordNotFound(t *testing.T) { @@ -1503,6 +1505,8 @@ func TestPGXMock_TransitionRIExchangeStatus_RecordNotFound(t *testing.T) { _, err := store.TransitionRIExchangeStatus(ctx, "ri-id", "pending", "processing", nil) require.Error(t, err) assert.Contains(t, err.Error(), "not found") + assert.NotErrorIs(t, err, ErrRIExchangeExpired) + assert.NotErrorIs(t, err, ErrRIExchangeNotInExpectedStatus) } func TestPGXMock_TransitionRIExchangeStatus_Expired(t *testing.T) { @@ -1521,6 +1525,8 @@ func TestPGXMock_TransitionRIExchangeStatus_Expired(t *testing.T) { _, err := store.TransitionRIExchangeStatus(ctx, "ri-id", "pending", "processing", nil) require.Error(t, err) assert.Contains(t, err.Error(), "expired") + assert.ErrorIs(t, err, ErrRIExchangeExpired, "the API maps this to 410 (issue #587)") + assert.NotErrorIs(t, err, ErrRIExchangeNotInExpectedStatus) } func TestPGXMock_TransitionRIExchangeStatus_Success(t *testing.T) { diff --git a/internal/config/store_postgres_ri_exchange_transition_integration_test.go b/internal/config/store_postgres_ri_exchange_transition_integration_test.go new file mode 100644 index 000000000..2104613aa --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_transition_integration_test.go @@ -0,0 +1,117 @@ +//go:build integration + +package config + +import ( + "context" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The RI exchange approve and reject links answer an expired exchange with +// 410 and an exchange another writer already moved with 409 (issue #587). +// Both codes come from the sentinel errors TransitionRIExchangeStatus wraps +// when its compare-and-set matches no row, so these tests pin them on a real +// PostgreSQL: the expiry predicate and the row lock that serializes +// concurrent transitions only exist in the database. + +func saveTransitionTestExchange(ctx context.Context, t *testing.T, store *PostgresStore, expiresAt time.Time) string { + t.Helper() + record := &RIExchangeRecord{ + AccountID: "123456789012", + ExchangeID: "transition-test-exchange", + Region: "us-east-1", + SourceRIIDs: []string{"ri-1"}, + SourceInstanceType: "r5.large", + SourceCount: 1, + TargetOfferingID: "off-1", + TargetInstanceType: "r5.xlarge", + TargetCount: 1, + Status: "pending", + Mode: "manual", + ExpiresAt: &expiresAt, + } + require.NoError(t, store.SaveRIExchangeRecord(ctx, record)) + return record.ID +} + +func TestPostgresStoreDB_TransitionRIExchangeStatus_ExpiredIsErrRIExchangeExpired(t *testing.T) { + conn := setupTestContainerDB(t) + store := NewPostgresStore(conn) + ctx := context.Background() + + id := saveTransitionTestExchange(ctx, t, store, time.Now().Add(-time.Hour)) + + for _, to := range []string{"processing", StatusCanceled} { + got, err := store.TransitionRIExchangeStatus(ctx, id, "pending", to, nil) + require.Error(t, err, "an expired exchange must not transition to %s", to) + assert.Nil(t, got) + assert.ErrorIs(t, err, ErrRIExchangeExpired) + assert.NotErrorIs(t, err, ErrRIExchangeNotInExpectedStatus) + } + + record, err := store.GetRIExchangeRecord(ctx, id) + require.NoError(t, err) + assert.Equal(t, "pending", record.Status, "a refused transition must leave the row alone") +} + +// TestPostgresStoreDB_TransitionRIExchangeStatus_ConcurrentLosersGetNotInExpectedStatus +// races approvals (pending -> processing) against rejections (pending -> +// canceled) on one unexpired row. Exactly one wins; every loser must get +// ErrRIExchangeNotInExpectedStatus (409), never a bare error (500) or the +// expiry sentinel (410). +func TestPostgresStoreDB_TransitionRIExchangeStatus_ConcurrentLosersGetNotInExpectedStatus(t *testing.T) { + conn := setupTestContainerDB(t) + store := NewPostgresStore(conn) + ctx := context.Background() + + const callers = 8 + for round := 0; round < 5; round++ { + id := saveTransitionTestExchange(ctx, t, store, time.Now().Add(time.Hour)) + + start := make(chan struct{}) + type outcome struct { + to string + rec *RIExchangeRecord + err error + } + results := make(chan outcome, callers) + var wg sync.WaitGroup + for i := 0; i < callers; i++ { + to := "processing" + if i%2 == 1 { + to = StatusCanceled + } + wg.Add(1) + go func(to string) { + defer wg.Done() + <-start + rec, err := store.TransitionRIExchangeStatus(ctx, id, "pending", to, nil) + results <- outcome{to: to, rec: rec, err: err} + }(to) + } + close(start) + wg.Wait() + close(results) + + var winners []string + for r := range results { + if r.err == nil { + require.NotNil(t, r.rec) + winners = append(winners, r.to) + continue + } + assert.ErrorIs(t, r.err, ErrRIExchangeNotInExpectedStatus, "round %d: a lost race must map to 409", round) + assert.NotErrorIs(t, r.err, ErrRIExchangeExpired, "round %d", round) + } + require.Len(t, winners, 1, "round %d: exactly one transition must win", round) + + record, err := store.GetRIExchangeRecord(ctx, id) + require.NoError(t, err) + assert.Equal(t, winners[0], record.Status, "round %d", round) + } +}