diff --git a/frontend/src/api/index.ts b/frontend/src/api/index.ts index 62f4524af..82802abd7 100644 --- a/frontend/src/api/index.ts +++ b/frontend/src/api/index.ts @@ -184,6 +184,7 @@ export { updateRIExchangeConfig, getRIExchangeHistory, listTargetOfferings, + approveRIExchange } from './riexchange'; // Re-export registrations functions and types diff --git a/frontend/src/api/riexchange.ts b/frontend/src/api/riexchange.ts index c3366282b..15bc41b14 100644 --- a/frontend/src/api/riexchange.ts +++ b/frontend/src/api/riexchange.ts @@ -107,3 +107,13 @@ export async function getRIExchangeHistory( const resp = await apiRequest<{ records: RIExchangeHistoryRecord[] }>(`/ri-exchange/history${qs}`); return resp.records ?? []; } + +/** + * Approve a pending RI exchange via session auth (issue #300). + * Mirrors the purchase approvePurchase API method. + */ +export async function approveRIExchange(id: string): Promise { + await apiRequest(`/ri-exchange/approve/${encodeURIComponent(id)}`, { + method: 'POST', + }); +} diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index f9194ab0d..c317491ba 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -634,6 +634,10 @@ export interface RIExchangeHistoryRecord { updated_at: string; completed_at?: string; expires_at?: string; + /** UUID of the session user who submitted this exchange (issue #300). */ + created_by_user_id?: string; + /** Email of the session user who approved via the dashboard Approve button (issue #300). */ + approved_by?: string; } // Inventory & Coverage types (issue #340 deferred sub-task — Active commitments) diff --git a/frontend/src/riexchange.ts b/frontend/src/riexchange.ts index aee5f2cb0..68df630eb 100644 --- a/frontend/src/riexchange.ts +++ b/frontend/src/riexchange.ts @@ -20,6 +20,8 @@ import type { import { openModal, closeModal } from './modal'; import { showSkeletonRows, teardownSkeleton } from './lib/skeleton'; import { canAccess } from './permissions'; +import { showToast } from './toast'; +import { getCurrentUser } from './state'; // Module state let currentRIs: ConvertibleRI[] = []; @@ -998,6 +1000,25 @@ export async function saveAutomationSettings(): Promise { // Exchange History // ────────────────────────────────────────────── +// canApproveRIExchangeRow returns true when the current session may approve +// the given pending RI exchange via the inline Approve button (issue #300). +// UX gate only — the backend authorizeSessionApproveRIExchange remains the +// security boundary; a false-positive here surfaces as a 403 toast on click. +// +// Heuristic mirrors canApprovePendingRow in history.ts: +// * status must be "pending"; +// * admin -> always yes; +// * non-admin matching created_by_user_id -> yes (approve-own); +// * legacy rows without created_by_user_id -> no (email-link path only). +function canApproveRIExchangeRow(rec: RIExchangeHistoryRecord): boolean { + if ((rec.status || '').toLowerCase() !== 'pending') return false; + const user = getCurrentUser(); + if (!user) return false; + if (canAccess('admin', '*') || canAccess('approve-any', 'purchases')) return true; + if (!rec.created_by_user_id) return false; + return canAccess('approve-own', 'purchases') && rec.created_by_user_id === user.id; +} + async function loadExchangeHistory(): Promise { const container = document.getElementById('ri-exchange-history-list'); if (!container) return; @@ -1047,6 +1068,9 @@ function renderExchangeHistory(container: HTMLElement, records: RIExchangeHistor const exchangeIdCell = rec.exchange_id ? '' + escapeHtml(rec.exchange_id) + '' : '—'; + const approveBtn = canApproveRIExchangeRow(rec) + ? '' + : ''; return '' + '' + escapeHtml(formatDateTime(rec.created_at)) + '' + '' + escapeHtml(String(rec.source_count)) + 'x ' + escapeHtml(rec.source_instance_type) + '' @@ -1055,12 +1079,13 @@ function renderExchangeHistory(container: HTMLElement, records: RIExchangeHistor + '$' + escapeHtml(rec.payment_due) + '' + '' + escapeHtml(rec.status) + '' + '' + exchangeIdCell + '' + + '' + approveBtn + '' + ''; }).join(''); const tableHTML = '' + '' - + '' + + '' + '' + '' + rowsHTML + '' + '
DateSource TypeTarget TypeCountPaymentStatusExchange IDDateSource TypeTarget TypeCountPaymentStatusExchange IDActions
'; @@ -1071,4 +1096,38 @@ function renderExchangeHistory(container: HTMLElement, records: RIExchangeHistor while (wrapper.firstChild) { container.appendChild(wrapper.firstChild); } + + // Wire Approve button click handlers + container.querySelectorAll('.riexchange-approve-btn[data-approve-id]').forEach(btn => { + btn.addEventListener('click', () => handleRIExchangeApproveClick(btn)); + }); +} + +async function handleRIExchangeApproveClick(btn: HTMLButtonElement): Promise { + const id = btn.dataset.approveId; + if (!id) return; + + const confirmed = await confirmDialog({ + title: 'Approve RI Exchange', + body: 'Approve this pending RI exchange? The exchange will execute immediately.', + confirmLabel: 'Approve', + }); + if (!confirmed) return; + + btn.disabled = true; + try { + await api.approveRIExchange(id); + showToast({ kind: 'success', message: 'RI exchange approved and executing.' }); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + showToast({ kind: 'error', message: 'Failed to approve exchange: ' + msg }); + btn.disabled = false; + return; + } + try { + await loadExchangeHistory(); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + showToast({ kind: 'error', message: 'Approved, but failed to refresh history: ' + msg }); + } } diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index 646466b48..8ed9b29ae 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -245,6 +245,9 @@ func (m *mockConfigStore) CompleteRIExchange(ctx context.Context, id string, exc func (m *mockConfigStore) FailRIExchange(ctx context.Context, id string, errorMsg string) error { return nil } +func (m *mockConfigStore) StampRIExchangeApprovedBy(ctx context.Context, id string, approverEmail string) error { + return nil +} func (m *mockConfigStore) GetRIExchangeDailySpend(ctx context.Context, date time.Time) (string, error) { return "0", nil } diff --git a/internal/api/coverage_extras_test.go b/internal/api/coverage_extras_test.go index 16d71b7e6..f68acea5c 100644 --- a/internal/api/coverage_extras_test.go +++ b/internal/api/coverage_extras_test.go @@ -366,7 +366,7 @@ func TestHandler_approveRIExchange_AlreadyProcessed(t *testing.T) { Return(nil, nil) h := &Handler{config: mockStore} - _, err := h.approveRIExchange(ctx, "11111111-1111-1111-1111-111111111111", "tok") + _, err := h.approveRIExchange(ctx, &events.LambdaFunctionURLRequest{}, "11111111-1111-1111-1111-111111111111", "tok") assert.Error(t, err) assert.Contains(t, err.Error(), "already processed") } diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index f21b2457f..5752e789b 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -773,14 +773,58 @@ func (h *Handler) getRIExchangeHistory(ctx context.Context, req *events.LambdaFu return &RIExchangeHistoryResponse{Records: records}, nil } -// approveRIExchange handles approval of a pending RI exchange via token. -func (h *Handler) approveRIExchange(ctx context.Context, id, token string) (any, error) { +// approveRIExchange handles approval of a pending RI exchange. +// +// Three-mode dispatch mirroring approvePurchase (issue #286, issue #300): +// +// 1. Session present AND RBAC-authorized (admin / approve-any / approve-own +// match) -> session-authed approve, regardless of whether a token is also +// in the URL. Closes issue #300. +// 2. token != "" -> legacy email-link flow. validateExchangeApproval enforces +// the token-equality check; the permission-denied fall-through ensures a +// logged-in user without approve-* can still use an email link they hold. +// 3. token == "" AND no qualifying session -> 403 via +// approveRIExchangeViaSession's requireSession gate. +func (h *Handler) approveRIExchange(ctx context.Context, req *events.LambdaFunctionURLRequest, id, token string) (any, error) { + if session := h.tryGetSession(ctx, req); session != nil { + // Quick RBAC pre-check (no record fetch needed): does this session hold + // ANY approve right? If yes, hand off to approveRIExchangeViaSession + // which will re-check ownership with the actual record. If 403, fall + // through to the token branch so email-link holders can still approve. + switch err := h.sessionHasApproveRight(ctx, session); { + case err == nil: + result, sessErr := h.approveRIExchangeViaSession(ctx, req, id, session) + if sessErr == nil { + return result, nil + } + // Record-level RBAC denied (e.g. approve-own user is not the creator). + // If a token is present, preserve legacy token flow; otherwise surface the error. + if !(token != "" && isPermissionDenied(sessErr)) { + return nil, sessErr + } + case isPermissionDenied(err): + // Logged-in user without approve-* may still hold a valid email token. + default: + return nil, err + } + } + + if token != "" { + return h.approveRIExchangeViaToken(ctx, id, token) + } + + return h.approveRIExchangeViaSession(ctx, req, id, nil) +} + +// approveRIExchangeViaToken is the legacy email-link branch of approveRIExchange. +// It validates the approval token, transitions the exchange to processing, and +// executes it. Extracted to keep approveRIExchange within cyclomatic-complexity limits. +func (h *Handler) approveRIExchangeViaToken(ctx context.Context, id, token string) (any, error) { record, err := h.validateExchangeApproval(ctx, id, token) if err != nil { return nil, err } - // Atomic transition: pending -> processing (checks expiry in WHERE clause) transitioned, err := h.config.TransitionRIExchangeStatus(ctx, id, "pending", "processing") if err != nil { return nil, fmt.Errorf("failed to transition exchange status: %w", err) @@ -792,6 +836,143 @@ func (h *Handler) approveRIExchange(ctx context.Context, id, token string) (any, return h.executeApprovedExchange(ctx, id, record) } +// approveRIExchangeViaSession is the session-authed branch of approveRIExchange +// (issue #300). Enforces the approve-any/approve-own RBAC matrix, then atomically +// transitions pending -> processing and executes the exchange. Stamps +// session.Email onto the approved_by column as an audit trail. +// +// The session parameter may be non-nil (already validated by the caller) or nil +// (requireSession will validate it and return 401 if absent). +func (h *Handler) approveRIExchangeViaSession(ctx context.Context, req *events.LambdaFunctionURLRequest, id string, session *Session) (any, error) { + var err error + if session == nil { + session, err = h.requireSession(ctx, req) + if err != nil { + return nil, err + } + } + + record, err := h.fetchAndAuthorizeRIExchange(ctx, session, id) + if err != nil { + return nil, err + } + + transitioned, err := h.config.TransitionRIExchangeStatus(ctx, id, "pending", "processing") + if err != nil { + return nil, fmt.Errorf("failed to transition exchange status: %w", err) + } + if transitioned == nil { + return nil, NewClientError(409, "exchange already processed, expired, or was cancelled by a newer analysis run") + } + + result, execErr := h.executeApprovedExchange(ctx, id, record) + + // Stamp approver attribution (best-effort: the exchange itself already + // executed, so a stamp failure is logged but not surfaced to the caller). + if execErr == nil { + if stampErr := h.config.StampRIExchangeApprovedBy(ctx, id, session.Email); stampErr != nil { + logging.Errorf("failed to stamp approved_by on exchange %s: %v", id, stampErr) + } + } + + return result, execErr +} + +// fetchAndAuthorizeRIExchange looks up the pending exchange record by id, checks +// that it is in "pending" state, and then verifies that session is authorised to +// approve it. Extracted from approveRIExchangeViaSession to keep that function +// under the cyclomatic-complexity limit. +func (h *Handler) fetchAndAuthorizeRIExchange(ctx context.Context, session *Session, id string) (*config.RIExchangeRecord, error) { + if err := validateUUID(id); err != nil { + return nil, err + } + + record, err := h.config.GetRIExchangeRecord(ctx, id) + if err != nil { + return nil, fmt.Errorf("failed to look up exchange record: %w", err) + } + if record == nil { + return nil, NewClientError(404, "exchange record not found") + } + + if record.Status != "pending" { + return nil, NewClientError(409, fmt.Sprintf("exchange %s cannot be approved (status=%s)", id, record.Status)) + } + + if err := h.authorizeSessionApproveRIExchange(ctx, session, record); err != nil { + return nil, err + } + + return record, nil +} + +// sessionHasApproveRight returns nil when the session holds ANY approve right +// on purchases (admin / approve-any / approve-own) without checking ownership. +// Used by the three-mode dispatch in approveRIExchange to decide whether to route +// to approveRIExchangeViaSession before fetching the record. +func (h *Handler) sessionHasApproveRight(ctx context.Context, session *Session) error { + if session.Role == "admin" { + return nil + } + if h.auth == nil { + return NewClientError(500, "authentication service not configured") + } + hasAny, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionApproveAny, auth.ResourcePurchases) + if err != nil { + return fmt.Errorf("permission check failed: %w", err) + } + if hasAny { + return nil + } + hasOwn, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionApproveOwn, auth.ResourcePurchases) + if err != nil { + return fmt.Errorf("permission check failed: %w", err) + } + if hasOwn { + return nil + } + return NewClientError(403, "permission denied: requires approve-any or approve-own on purchases") +} + +// authorizeSessionApproveRIExchange returns nil when the session is permitted to +// approve the given RI exchange record under the approve-any / approve-own RBAC rules +// (issue #300). Mirrors authorizeSessionApprove from handler_purchases.go. +// +// The RI exchange shares ResourcePurchases because approval is conceptually +// "approving a purchase action on a different resource type" per the issue spec, +// which prefers reusing the existing verbs to keep the matrix small. +func (h *Handler) authorizeSessionApproveRIExchange(ctx context.Context, session *Session, record *config.RIExchangeRecord) error { + if session.Role == "admin" { + return nil + } + if h.auth == nil { + return NewClientError(500, "authentication service not configured") + } + + hasAny, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionApproveAny, auth.ResourcePurchases) + if err != nil { + return fmt.Errorf("permission check failed: %w", err) + } + if hasAny { + return nil + } + + hasOwn, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionApproveOwn, auth.ResourcePurchases) + if err != nil { + return fmt.Errorf("permission check failed: %w", err) + } + if !hasOwn { + return NewClientError(403, "permission denied: requires approve-any or approve-own on purchases") + } + + // approve-own: only allow if the session user created this exchange. + if record.CreatedByUserID == nil || *record.CreatedByUserID != session.UserID { + return NewClientError(403, "permission denied: cannot approve another user's pending exchange") + } + + return nil +} + // validateExchangeApproval validates ID, token, and record state for an exchange approval. func (h *Handler) validateExchangeApproval(ctx context.Context, id, token string) (*config.RIExchangeRecord, error) { if err := validateUUID(id); err != nil { diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 4eb0709f8..5fcfd0fdd 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/LeanerCloud/CUDly/internal/auth" "github.com/LeanerCloud/CUDly/internal/config" "github.com/LeanerCloud/CUDly/providers/aws/recommendations" ec2svc "github.com/LeanerCloud/CUDly/providers/aws/services/ec2" @@ -250,7 +251,7 @@ func TestApproveRIExchange_AlreadyCancelled(t *testing.T) { mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing"). Return((*config.RIExchangeRecord)(nil), nil) - _, err := h.approveRIExchange(ctx, id, token) + _, err := h.approveRIExchange(ctx, &events.LambdaFunctionURLRequest{}, id, token) assert.Error(t, err) ce, ok := IsClientError(err) assert.True(t, ok) @@ -280,7 +281,7 @@ func TestApproveRIExchange_DoubleApprove(t *testing.T) { mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing"). Return((*config.RIExchangeRecord)(nil), nil) - _, err := h.approveRIExchange(ctx, id, token) + _, err := h.approveRIExchange(ctx, &events.LambdaFunctionURLRequest{}, id, token) assert.Error(t, err) ce, ok := IsClientError(err) assert.True(t, ok) @@ -302,7 +303,7 @@ func TestApproveRIExchange_InvalidToken(t *testing.T) { Status: "pending", }, nil) - _, err := h.approveRIExchange(ctx, id, "wrong-token") + _, err := h.approveRIExchange(ctx, &events.LambdaFunctionURLRequest{}, id, "wrong-token") assert.Error(t, err) ce, ok := IsClientError(err) assert.True(t, ok) @@ -310,6 +311,165 @@ func TestApproveRIExchange_InvalidToken(t *testing.T) { assert.Contains(t, err.Error(), "invalid approval token") } +// TestApproveRIExchange_SessionAdmin verifies that an admin session can approve +// a pending RI exchange without an email token (issue #300). +func TestApproveRIExchange_SessionAdmin(t *testing.T) { + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + h := &Handler{config: mockStore, auth: mockAuth} + ctx := context.Background() + id := "550e8400-e29b-41d4-a716-446655440010" + creatorID := "creator-uuid" + + adminSession := &Session{UserID: "admin-uuid", Email: "admin@example.com", Role: "admin"} + mockAuth.On("ValidateSession", ctx, "admin-bearer").Return(adminSession, nil) + + // authorizeSessionApproveRIExchange: admin role short-circuits (no HasPermissionAPI call) + + // approveRIExchangeViaSession fetches the record to check status + mockStore.On("GetRIExchangeRecord", ctx, id).Return(&config.RIExchangeRecord{ + ID: id, + Status: "pending", + ApprovalToken: "tok", + SourceRIIDs: []string{"ri-123"}, + PaymentDue: "100.00", + CreatedByUserID: &creatorID, + }, nil).Once() + + mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing"). + Return(&config.RIExchangeRecord{ID: id, Status: "processing", SourceRIIDs: []string{"ri-123"}, PaymentDue: "100.00"}, nil) + // executeApprovedExchange calls GetRIExchangeDailySpend + GetGlobalConfig + mockStore.On("GetRIExchangeDailySpend", mock.Anything, mock.Anything).Return("0", nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + RIExchangeMaxDailyUSD: 1000, + RIExchangeMaxPerExchangeUSD: 500, + }, nil) + // executeApprovedExchange will call failExchange (no real AWS SDK) which + // returns a non-error result map. Since execErr == nil, StampRIExchangeApprovedBy + // is called as best-effort audit trail. + mockStore.On("FailRIExchange", ctx, id, mock.AnythingOfType("string")).Return(nil) + mockStore.On("StampRIExchangeApprovedBy", ctx, id, adminSession.Email).Return(nil) + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer admin-bearer"}, + } + _, err := h.approveRIExchange(ctx, req, id, "") + require.NoError(t, err) + // The exchange execution will fail (no real AWS SDK) but we verify the + // dispatch reached the session-authed path. + mockStore.AssertCalled(t, "GetRIExchangeRecord", ctx, id) + mockStore.AssertCalled(t, "TransitionRIExchangeStatus", ctx, id, "pending", "processing") + mockStore.AssertCalled(t, "FailRIExchange", ctx, id, mock.AnythingOfType("string")) + mockStore.AssertCalled(t, "StampRIExchangeApprovedBy", ctx, id, adminSession.Email) +} + +// TestApproveRIExchange_SessionApproveOwn verifies that a user with approve-own +// can approve a pending exchange they created, but not one created by another user. +func TestApproveRIExchange_SessionApproveOwn(t *testing.T) { + ctx := context.Background() + id := "550e8400-e29b-41d4-a716-446655440011" + ownerID := "owner-uuid" + otherID := "other-uuid" + + t.Run("own exchange allowed", func(t *testing.T) { + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + h := &Handler{config: mockStore, auth: mockAuth} + + ownerSession := &Session{UserID: ownerID, Email: "owner@example.com", Role: "user"} + mockAuth.On("ValidateSession", ctx, "owner-bearer").Return(ownerSession, nil) + mockAuth.On("HasPermissionAPI", ctx, ownerID, auth.ActionApproveAny, auth.ResourcePurchases).Return(false, nil) + mockAuth.On("HasPermissionAPI", ctx, ownerID, auth.ActionApproveOwn, auth.ResourcePurchases).Return(true, nil) + + // authorizeSessionApproveRIExchange fetches record for ownership check + mockStore.On("GetRIExchangeRecord", ctx, id).Return(&config.RIExchangeRecord{ + ID: id, + Status: "pending", + SourceRIIDs: []string{"ri-1"}, + PaymentDue: "50.00", + ApprovalToken: "tok", + CreatedByUserID: &ownerID, + }, nil) + + mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing"). + Return(&config.RIExchangeRecord{ID: id, Status: "processing", SourceRIIDs: []string{"ri-1"}, PaymentDue: "50.00"}, nil) + mockStore.On("GetRIExchangeDailySpend", mock.Anything, mock.Anything).Return("0", nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + RIExchangeMaxDailyUSD: 1000, + RIExchangeMaxPerExchangeUSD: 500, + }, nil) + mockStore.On("FailRIExchange", ctx, id, mock.AnythingOfType("string")).Return(nil) + mockStore.On("StampRIExchangeApprovedBy", ctx, id, ownerSession.Email).Return(nil) + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer owner-bearer"}, + } + _, err := h.approveRIExchange(ctx, req, id, "") + require.NoError(t, err) + mockStore.AssertCalled(t, "TransitionRIExchangeStatus", ctx, id, "pending", "processing") + }) + + t.Run("other user exchange rejected", func(t *testing.T) { + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + h := &Handler{config: mockStore, auth: mockAuth} + + ownerSession := &Session{UserID: ownerID, Email: "owner@example.com", Role: "user"} + mockAuth.On("ValidateSession", ctx, "owner-bearer").Return(ownerSession, nil) + mockAuth.On("HasPermissionAPI", ctx, ownerID, auth.ActionApproveAny, auth.ResourcePurchases).Return(false, nil) + mockAuth.On("HasPermissionAPI", ctx, ownerID, auth.ActionApproveOwn, auth.ResourcePurchases).Return(true, nil) + + // authorizeSessionApproveRIExchange fetches record — creator does not match + mockStore.On("GetRIExchangeRecord", ctx, id).Return(&config.RIExchangeRecord{ + ID: id, + Status: "pending", + ApprovalToken: "tok", + CreatedByUserID: &otherID, + }, nil) + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer owner-bearer"}, + } + _, err := h.approveRIExchange(ctx, req, id, "") + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + assert.Contains(t, err.Error(), "cannot approve another user's pending exchange") + }) +} + +// TestApproveRIExchange_LegacyTokenStillWorks verifies that the token-only path +// continues to work for non-session callers after the dual-auth refactor (backwards-compat). +func TestApproveRIExchange_LegacyTokenStillWorks(t *testing.T) { + mockStore := new(MockConfigStore) + h := &Handler{config: mockStore} // no auth configured -> tryGetSession returns nil + ctx := context.Background() + id := "550e8400-e29b-41d4-a716-446655440012" + token := "legacy-token" + + mockStore.On("GetRIExchangeRecord", ctx, id).Return(&config.RIExchangeRecord{ + ID: id, + ApprovalToken: token, + Status: "pending", + SourceRIIDs: []string{"ri-1"}, + PaymentDue: "10.00", + }, nil) + mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing"). + Return(&config.RIExchangeRecord{ID: id, Status: "processing"}, nil) + mockStore.On("GetRIExchangeDailySpend", mock.Anything, mock.Anything).Return("0", nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + RIExchangeMaxDailyUSD: 1000, + RIExchangeMaxPerExchangeUSD: 500, + }, nil) + mockStore.On("FailRIExchange", ctx, id, mock.AnythingOfType("string")).Return(nil) + + req := &events.LambdaFunctionURLRequest{} + _, err := h.approveRIExchange(ctx, req, id, token) + require.NoError(t, err) + mockStore.AssertCalled(t, "TransitionRIExchangeStatus", ctx, id, "pending", "processing") +} + func TestRejectRIExchange_MissingToken(t *testing.T) { h := &Handler{} ctx := context.Background() diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index f9bf700d1..8fb7a1249 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -299,6 +299,11 @@ func (m *MockConfigStore) CompleteRIExchange(ctx context.Context, id string, exc return args.Error(0) } +func (m *MockConfigStore) StampRIExchangeApprovedBy(ctx context.Context, id string, approverEmail string) error { + args := m.Called(ctx, id, approverEmail) + return args.Error(0) +} + func (m *MockConfigStore) FailRIExchange(ctx context.Context, id string, errorMsg string) error { args := m.Called(ctx, id, errorMsg) return args.Error(0) diff --git a/internal/api/router.go b/internal/api/router.go index 4ad2e6b05..0447a89a2 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -727,7 +727,7 @@ func (r *Router) approveRIExchangeHandler(ctx context.Context, req *events.Lambd if err := r.h.checkRateLimit(ctx, req, "approve_cancel_public"); err != nil { return nil, err } - return r.h.approveRIExchange(ctx, params["id"], req.QueryStringParameters["token"]) + return r.h.approveRIExchange(ctx, req, params["id"], req.QueryStringParameters["token"]) } func (r *Router) rejectRIExchangeHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 83c9d233d..457eeef19 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -110,6 +110,10 @@ type StoreInterface interface { GetRIExchangeHistory(ctx context.Context, since time.Time, limit int) ([]RIExchangeRecord, error) TransitionRIExchangeStatus(ctx context.Context, id string, fromStatus string, toStatus string) (*RIExchangeRecord, error) CompleteRIExchange(ctx context.Context, id string, exchangeID string) error + // StampRIExchangeApprovedBy sets the approved_by column on a completed + // exchange row (issue #300). Called after CompleteRIExchange when the + // approval came from a session-authed user rather than an email token. + StampRIExchangeApprovedBy(ctx context.Context, id string, approverEmail string) error FailRIExchange(ctx context.Context, id string, errorMsg string) error GetRIExchangeDailySpend(ctx context.Context, date time.Time) (string, error) CancelAllPendingExchanges(ctx context.Context) (int64, error) diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 7f0c77d9d..6905f4105 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1480,8 +1480,8 @@ func (s *PostgresStore) SaveRIExchangeRecord(ctx context.Context, record *RIExch source_instance_type, source_count, target_offering_id, target_instance_type, target_count, payment_due, status, approval_token, error, mode, completed_at, expires_at, - created_at, updated_at - ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19) + created_at, updated_at, created_by_user_id + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20) ` _, err := s.db.Exec(ctx, query, @@ -1504,6 +1504,7 @@ func (s *PostgresStore) SaveRIExchangeRecord(ctx context.Context, record *RIExch record.ExpiresAt, record.CreatedAt, record.UpdatedAt, + record.CreatedByUserID, ) if err != nil { @@ -1520,7 +1521,8 @@ func (s *PostgresStore) GetRIExchangeRecord(ctx context.Context, id string) (*RI source_instance_type, source_count, target_offering_id, target_instance_type, target_count, payment_due::text, status, approval_token, error, mode, - created_at, updated_at, completed_at, expires_at + created_at, updated_at, completed_at, expires_at, + created_by_user_id, approved_by FROM ri_exchange_history WHERE id = $1 ` @@ -1544,7 +1546,8 @@ func (s *PostgresStore) GetRIExchangeRecordByToken(ctx context.Context, token st source_instance_type, source_count, target_offering_id, target_instance_type, target_count, payment_due::text, status, approval_token, error, mode, - created_at, updated_at, completed_at, expires_at + created_at, updated_at, completed_at, expires_at, + created_by_user_id, approved_by FROM ri_exchange_history WHERE approval_token = $1 ` @@ -1568,7 +1571,8 @@ func (s *PostgresStore) GetRIExchangeHistory(ctx context.Context, since time.Tim source_instance_type, source_count, target_offering_id, target_instance_type, target_count, payment_due::text, status, approval_token, error, mode, - created_at, updated_at, completed_at, expires_at + created_at, updated_at, completed_at, expires_at, + created_by_user_id, approved_by FROM ri_exchange_history WHERE created_at >= $1 ORDER BY created_at DESC @@ -1590,7 +1594,8 @@ func (s *PostgresStore) TransitionRIExchangeStatus(ctx context.Context, id strin source_instance_type, source_count, target_offering_id, target_instance_type, target_count, payment_due::text, status, approval_token, error, mode, - created_at, updated_at, completed_at, expires_at + created_at, updated_at, completed_at, expires_at, + created_by_user_id, approved_by ` records, err := s.queryRIExchangeRecords(ctx, query, id, fromStatus, toStatus) @@ -1645,6 +1650,27 @@ func (s *PostgresStore) CompleteRIExchange(ctx context.Context, id string, excha return nil } +// StampRIExchangeApprovedBy sets the approved_by column on an RI exchange row +// (issue #300). Called after CompleteRIExchange when approval came from a +// session-authed user. The stamping is best-effort (log + continue on failure +// so the exchange itself isn't rolled back just because the audit stamp failed). +func (s *PostgresStore) StampRIExchangeApprovedBy(ctx context.Context, id string, approverEmail string) error { + query := ` + UPDATE ri_exchange_history + SET approved_by = $2 + WHERE id = $1 + ` + + result, err := s.db.Exec(ctx, query, id, approverEmail) + if err != nil { + return fmt.Errorf("failed to stamp approved_by on ri exchange %s: %w", id, err) + } + if result.RowsAffected() == 0 { + return fmt.Errorf("ri exchange record not found when stamping approved_by: %s", id) + } + return nil +} + // FailRIExchange marks an RI exchange as failed func (s *PostgresStore) FailRIExchange(ctx context.Context, id string, errorMsg string) error { query := ` @@ -1707,7 +1733,8 @@ func (s *PostgresStore) GetStaleProcessingExchanges(ctx context.Context, olderTh source_instance_type, source_count, target_offering_id, target_instance_type, target_count, payment_due::text, status, approval_token, error, mode, - created_at, updated_at, completed_at, expires_at + created_at, updated_at, completed_at, expires_at, + created_by_user_id, approved_by FROM ri_exchange_history WHERE status = 'processing' AND updated_at < NOW() - $1::interval ` @@ -1728,6 +1755,7 @@ func (s *PostgresStore) queryRIExchangeRecords(ctx context.Context, query string var record RIExchangeRecord var approvalToken, errStr sql.NullString var completedAt, expiresAt sql.NullTime + var createdByUserID, approvedBy sql.NullString err := rows.Scan( &record.ID, @@ -1749,6 +1777,8 @@ func (s *PostgresStore) queryRIExchangeRecords(ctx context.Context, query string &record.UpdatedAt, &completedAt, &expiresAt, + &createdByUserID, + &approvedBy, ) if err != nil { return nil, fmt.Errorf("failed to scan ri exchange record: %w", err) @@ -1766,6 +1796,12 @@ func (s *PostgresStore) queryRIExchangeRecords(ctx context.Context, query string if expiresAt.Valid { record.ExpiresAt = &expiresAt.Time } + if createdByUserID.Valid { + record.CreatedByUserID = &createdByUserID.String + } + if approvedBy.Valid { + record.ApprovedBy = &approvedBy.String + } records = append(records, record) } diff --git a/internal/config/store_postgres_nil_db_test.go b/internal/config/store_postgres_nil_db_test.go index 19a3cdf62..9773bdadf 100644 --- a/internal/config/store_postgres_nil_db_test.go +++ b/internal/config/store_postgres_nil_db_test.go @@ -129,6 +129,18 @@ func TestPostgresStore_FailRIExchange_NilDB(t *testing.T) { assert.True(t, panicked, "expected panic with nil db connection") } +// TestPostgresStore_StampRIExchangeApprovedBy_NilDB exercises the method entry. +func TestPostgresStore_StampRIExchangeApprovedBy_NilDB(t *testing.T) { + store := NewPostgresStore(nil) + ctx := context.Background() + + panicked := callWithRecover(func() { + _ = store.StampRIExchangeApprovedBy(ctx, "ri-id", "approver@example.com") + }) + + assert.True(t, panicked, "expected panic with nil db connection") +} + // TestPostgresStore_GetRIExchangeDailySpend_NilDB exercises the method entry. func TestPostgresStore_GetRIExchangeDailySpend_NilDB(t *testing.T) { store := NewPostgresStore(nil) diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 9981a83be..f48076627 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -623,6 +623,7 @@ func riExchangeRow(now time.Time) []interface{} { sql.NullString{}, "manual", now, now, sql.NullTime{}, sql.NullTime{}, + sql.NullString{}, sql.NullString{}, // created_by_user_id, approved_by } } @@ -632,6 +633,7 @@ var riExchangeCols = []string{ "target_instance_type", "target_count", "payment_due", "status", "approval_token", "error", "mode", "created_at", "updated_at", "completed_at", "expires_at", + "created_by_user_id", "approved_by", } func TestPGXMock_GetRIExchangeRecord_Success(t *testing.T) { @@ -665,6 +667,7 @@ func TestPGXMock_GetRIExchangeRecord_WithTimestamps(t *testing.T) { sql.NullString{Valid: true, String: "some error"}, "auto", now, now, sql.NullTime{Valid: true, Time: now}, sql.NullTime{Valid: true, Time: now}, + sql.NullString{}, sql.NullString{}, // created_by_user_id, approved_by } rows := pgxmock.NewRows(riExchangeCols).AddRow(row...) mock.ExpectQuery("SELECT").WithArgs(pgxmock.AnyArg()).WillReturnRows(rows) diff --git a/internal/config/types.go b/internal/config/types.go index 5535b4f63..caf518f68 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -534,26 +534,34 @@ type PurchaseHistoryRecord struct { // RIExchangeRecord represents a record in the ri_exchange_history table type RIExchangeRecord struct { - ID string `json:"id"` - AccountID string `json:"account_id"` - ExchangeID string `json:"exchange_id"` - Region string `json:"region"` - SourceRIIDs []string `json:"source_ri_ids"` - SourceInstanceType string `json:"source_instance_type"` - SourceCount int `json:"source_count"` - TargetOfferingID string `json:"target_offering_id"` - TargetInstanceType string `json:"target_instance_type"` - TargetCount int `json:"target_count"` - PaymentDue string `json:"payment_due"` - Status string `json:"status"` - ApprovalToken string `json:"approval_token,omitempty"` - Error string `json:"error,omitempty"` - Mode string `json:"mode"` - CreatedAt time.Time `json:"created_at"` - UpdatedAt time.Time `json:"updated_at"` - CompletedAt *time.Time `json:"completed_at,omitempty"` - ExpiresAt *time.Time `json:"expires_at,omitempty"` - CloudAccountID *string `json:"cloud_account_id,omitempty"` + ID string `json:"id"` + AccountID string `json:"account_id"` + ExchangeID string `json:"exchange_id"` + Region string `json:"region"` + SourceRIIDs []string `json:"source_ri_ids"` + SourceInstanceType string `json:"source_instance_type"` + SourceCount int `json:"source_count"` + TargetOfferingID string `json:"target_offering_id"` + TargetInstanceType string `json:"target_instance_type"` + TargetCount int `json:"target_count"` + PaymentDue string `json:"payment_due"` + Status string `json:"status"` + ApprovalToken string `json:"approval_token,omitempty"` + Error string `json:"error,omitempty"` + Mode string `json:"mode"` + // CreatedByUserID is the UUID of the session user who submitted the exchange + // (populated for dashboard-initiated exchanges; nil for automated or legacy + // email-link-initiated ones). Exposed to the frontend so the Approve button + // can apply the approve-own ownership check client-side. + CreatedByUserID *string `json:"created_by_user_id,omitempty"` + // ApprovedBy carries the email of the session user who approved the exchange + // via the dashboard Approve button (issue #300). Nil for token-authed approvals. + ApprovedBy *string `json:"approved_by,omitempty"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` + CompletedAt *time.Time `json:"completed_at,omitempty"` + ExpiresAt *time.Time `json:"expires_at,omitempty"` + CloudAccountID *string `json:"cloud_account_id,omitempty"` } // ConfigSetting represents a configuration setting for the defaults system diff --git a/internal/database/postgres/migrations/000054_ri_exchange_approver_creator.down.sql b/internal/database/postgres/migrations/000054_ri_exchange_approver_creator.down.sql new file mode 100644 index 000000000..94397235e --- /dev/null +++ b/internal/database/postgres/migrations/000054_ri_exchange_approver_creator.down.sql @@ -0,0 +1,4 @@ +-- Revert migration 000054 +ALTER TABLE ri_exchange_history + DROP COLUMN IF EXISTS approved_by, + DROP COLUMN IF EXISTS created_by_user_id; diff --git a/internal/database/postgres/migrations/000054_ri_exchange_approver_creator.up.sql b/internal/database/postgres/migrations/000054_ri_exchange_approver_creator.up.sql new file mode 100644 index 000000000..5720b5a32 --- /dev/null +++ b/internal/database/postgres/migrations/000054_ri_exchange_approver_creator.up.sql @@ -0,0 +1,6 @@ +-- Migration 000054: add approved_by and created_by_user_id to ri_exchange_history +-- Mirrors the same columns on purchase_executions added for issues #286 and #46. +-- Both columns are nullable so existing rows are unaffected. +ALTER TABLE ri_exchange_history + ADD COLUMN IF NOT EXISTS approved_by TEXT, + ADD COLUMN IF NOT EXISTS created_by_user_id TEXT; diff --git a/internal/purchase/mocks_test.go b/internal/purchase/mocks_test.go index 9f85d0c39..e39bf8f0c 100644 --- a/internal/purchase/mocks_test.go +++ b/internal/purchase/mocks_test.go @@ -399,6 +399,11 @@ func (m *MockConfigStore) FailRIExchange(ctx context.Context, id string, errorMs return args.Error(0) } +func (m *MockConfigStore) StampRIExchangeApprovedBy(ctx context.Context, id string, approverEmail string) error { + args := m.Called(ctx, id, approverEmail) + return args.Error(0) +} + func (m *MockConfigStore) GetRIExchangeDailySpend(ctx context.Context, date time.Time) (string, error) { args := m.Called(ctx, date) return args.String(0), args.Error(1) diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 04170975d..bb827162f 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -279,6 +279,11 @@ func (m *MockConfigStore) FailRIExchange(ctx context.Context, id string, errorMs return args.Error(0) } +func (m *MockConfigStore) StampRIExchangeApprovedBy(ctx context.Context, id string, approverEmail string) error { + args := m.Called(ctx, id, approverEmail) + return args.Error(0) +} + func (m *MockConfigStore) GetRIExchangeDailySpend(ctx context.Context, date time.Time) (string, error) { args := m.Called(ctx, date) return args.String(0), args.Error(1) diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index effc3b193..913476803 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -144,6 +144,9 @@ func (m *mockConfigStoreForHealth) CompleteRIExchange(ctx context.Context, id st func (m *mockConfigStoreForHealth) FailRIExchange(ctx context.Context, id string, errorMsg string) error { return nil } +func (m *mockConfigStoreForHealth) StampRIExchangeApprovedBy(_ context.Context, _ string, _ string) error { + return nil +} func (m *mockConfigStoreForHealth) GetRIExchangeDailySpend(ctx context.Context, date time.Time) (string, error) { return "0", nil }