From 7293268a1301c4eaf8f09c392bc8d7218551b9e3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 20 May 2026 18:52:25 +0200 Subject: [PATCH 1/3] feat(ri-exchange): add symmetric session-auth approve path (closes #300) Mirror the three-mode dispatch introduced for purchases (#286): 1. Session with approve-any/approve-own on ResourcePurchases -> RBAC approve, stamping approved_by for audit trail. 2. Legacy email token -> unchanged behaviour. 3. No session and no token -> 401 via requireSession. Migration 000053 adds nullable approved_by and created_by_user_id to ri_exchange_history. The Approve button appears in the RI Exchange history panel for rows the current user is authorised to approve. --- frontend/src/api/index.ts | 1 + frontend/src/api/riexchange.ts | 10 + frontend/src/api/types.ts | 4 + frontend/src/riexchange.ts | 55 +++++- internal/analytics/collector_test.go | 3 + internal/api/coverage_extras_test.go | 2 +- internal/api/handler_ri_exchange.go | 176 +++++++++++++++++- internal/api/handler_ri_exchange_test.go | 163 +++++++++++++++- internal/api/mocks_test.go | 5 + internal/api/router.go | 2 +- internal/config/interfaces.go | 4 + internal/config/store_postgres.go | 50 ++++- internal/config/store_postgres_nil_db_test.go | 12 ++ .../config/store_postgres_pgxmock_test.go | 3 + internal/config/types.go | 48 +++-- ...0053_ri_exchange_approver_creator.down.sql | 4 + ...000053_ri_exchange_approver_creator.up.sql | 6 + internal/purchase/mocks_test.go | 5 + internal/scheduler/scheduler_test.go | 5 + internal/server/test_helpers_test.go | 3 + 20 files changed, 523 insertions(+), 38 deletions(-) create mode 100644 internal/database/postgres/migrations/000053_ri_exchange_approver_creator.down.sql create mode 100644 internal/database/postgres/migrations/000053_ri_exchange_approver_creator.up.sql 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..a33ff2e1f 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 (user.role === 'admin') return true; + if (!rec.created_by_user_id) return false; + return 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,32 @@ 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.' }); + await loadExchangeHistory(); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + showToast({ kind: 'error', message: 'Failed to approve exchange: ' + msg }); + btn.disabled = false; + } } 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..a516d7e00 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -773,14 +773,75 @@ 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) { - record, err := h.validateExchangeApproval(ctx, id, token) +// 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: + return h.approveRIExchangeViaSession(ctx, req, id, session) + case isPermissionDenied(err): + // Logged-in user without approve-* may still hold a valid email token. + default: + return nil, err + } + } + + if token != "" { + record, err := h.validateExchangeApproval(ctx, id, token) + 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") + } + + return h.executeApprovedExchange(ctx, id, record) + } + + return h.approveRIExchangeViaSession(ctx, req, id, nil) +} + +// 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 } - // 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) @@ -789,7 +850,112 @@ func (h *Handler) approveRIExchange(ctx context.Context, id, token string) (any, return nil, NewClientError(409, "exchange already processed, expired, or was cancelled by a newer analysis run") } - return h.executeApprovedExchange(ctx, id, record) + 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. diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 4eb0709f8..237252258 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,162 @@ 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"}, + } + _, _ = h.approveRIExchange(ctx, req, id, "") + // 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"}, + } + _, _ = h.approveRIExchange(ctx, req, id, "") + 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{} + _, _ = h.approveRIExchange(ctx, req, id, token) + 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/000053_ri_exchange_approver_creator.down.sql b/internal/database/postgres/migrations/000053_ri_exchange_approver_creator.down.sql new file mode 100644 index 000000000..0ac38d95e --- /dev/null +++ b/internal/database/postgres/migrations/000053_ri_exchange_approver_creator.down.sql @@ -0,0 +1,4 @@ +-- Revert migration 000053 +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/000053_ri_exchange_approver_creator.up.sql b/internal/database/postgres/migrations/000053_ri_exchange_approver_creator.up.sql new file mode 100644 index 000000000..e8e991d1e --- /dev/null +++ b/internal/database/postgres/migrations/000053_ri_exchange_approver_creator.up.sql @@ -0,0 +1,6 @@ +-- Migration 000053: 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 } From 681efb536868fea8a2fe2783f73c41bbe108579a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 27 May 2026 12:53:37 +0200 Subject: [PATCH 2/3] fix(ri-exchange): address CodeRabbit findings on PR #590 (review 4357903828) Finding 1 (Major): canApproveRIExchangeRow now uses canAccess() for both admin/* and approve-any:purchases holders; approve-own:purchases replaces the bare user-id equality check. Non-admin approve-any holders can now see and click the Approve button. Finding 2 (Minor): split approveRIExchange's single try/catch into two blocks so a refresh failure no longer triggers the "Failed to approve" error toast. Approval and history reload are independent failures. Finding 3 (Minor): three test call sites that silently discarded the returned err now capture it and assert require.NoError, ensuring regressions surface instead of passing on side-effect checks alone. Finding 4 (Major): when sessionHasApproveRight passes but approveRIExchangeViaSession returns permission-denied (approve-own user not the creator), fall through to the token path if a token is present instead of returning the 403. Legacy email-link approval now works for this RBAC shape. --- frontend/src/riexchange.ts | 12 +++++-- internal/api/handler_ri_exchange.go | 43 ++++++++++++++++-------- internal/api/handler_ri_exchange_test.go | 9 +++-- 3 files changed, 44 insertions(+), 20 deletions(-) diff --git a/frontend/src/riexchange.ts b/frontend/src/riexchange.ts index a33ff2e1f..68df630eb 100644 --- a/frontend/src/riexchange.ts +++ b/frontend/src/riexchange.ts @@ -1014,9 +1014,9 @@ function canApproveRIExchangeRow(rec: RIExchangeHistoryRecord): boolean { if ((rec.status || '').toLowerCase() !== 'pending') return false; const user = getCurrentUser(); if (!user) return false; - if (user.role === 'admin') return true; + if (canAccess('admin', '*') || canAccess('approve-any', 'purchases')) return true; if (!rec.created_by_user_id) return false; - return rec.created_by_user_id === user.id; + return canAccess('approve-own', 'purchases') && rec.created_by_user_id === user.id; } async function loadExchangeHistory(): Promise { @@ -1118,10 +1118,16 @@ async function handleRIExchangeApproveClick(btn: HTMLButtonElement): Promise Date: Wed, 27 May 2026 13:22:57 +0200 Subject: [PATCH 3/3] fix(migrations): renumber ri_exchange_approver_creator to 000054 After the rebase onto current feat/multicloud-web-frontend, the PR's new migration collided with the base branch's 000053_executions_ account_fk_restrict. Renumber via `git mv` to the next free slot (000054). Project memory `project_migration_number_collisions.md` covers this exact failure mode (pre-commit hook fails locally and in CI when two PRs both add migrations numbered the same after one of them lands first). - 000053_ri_exchange_approver_creator.up.sql -> 000054_* - 000053_ri_exchange_approver_creator.down.sql -> 000054_* - Updated the "Migration 000053" / "Revert migration 000053" comment headers inside each file to match the new number. Verified 000054 is free on origin/feat and no open PR introduces a clashing migration. --- ...or.down.sql => 000054_ri_exchange_approver_creator.down.sql} | 2 +- ...reator.up.sql => 000054_ri_exchange_approver_creator.up.sql} | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename internal/database/postgres/migrations/{000053_ri_exchange_approver_creator.down.sql => 000054_ri_exchange_approver_creator.down.sql} (81%) rename internal/database/postgres/migrations/{000053_ri_exchange_approver_creator.up.sql => 000054_ri_exchange_approver_creator.up.sql} (82%) diff --git a/internal/database/postgres/migrations/000053_ri_exchange_approver_creator.down.sql b/internal/database/postgres/migrations/000054_ri_exchange_approver_creator.down.sql similarity index 81% rename from internal/database/postgres/migrations/000053_ri_exchange_approver_creator.down.sql rename to internal/database/postgres/migrations/000054_ri_exchange_approver_creator.down.sql index 0ac38d95e..94397235e 100644 --- a/internal/database/postgres/migrations/000053_ri_exchange_approver_creator.down.sql +++ b/internal/database/postgres/migrations/000054_ri_exchange_approver_creator.down.sql @@ -1,4 +1,4 @@ --- Revert migration 000053 +-- 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/000053_ri_exchange_approver_creator.up.sql b/internal/database/postgres/migrations/000054_ri_exchange_approver_creator.up.sql similarity index 82% rename from internal/database/postgres/migrations/000053_ri_exchange_approver_creator.up.sql rename to internal/database/postgres/migrations/000054_ri_exchange_approver_creator.up.sql index e8e991d1e..5720b5a32 100644 --- a/internal/database/postgres/migrations/000053_ri_exchange_approver_creator.up.sql +++ b/internal/database/postgres/migrations/000054_ri_exchange_approver_creator.up.sql @@ -1,4 +1,4 @@ --- Migration 000053: add approved_by and created_by_user_id to ri_exchange_history +-- 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