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.

Author: cristim

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<void> {
@@ -1118,10 +1118,16 @@ async function handleRIExchangeApproveClick(btn: HTMLButtonElement): Promise<voi
   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;
+    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 });
   }
 }

internal/api/handler_ri_exchange.go

@@ -793,7 +793,15 @@ func (h *Handler) approveRIExchange(ctx context.Context, req *events.LambdaFunct
 		// 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)
+			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:
@@ -802,23 +810,30 @@ func (h *Handler) approveRIExchange(ctx context.Context, req *events.LambdaFunct
 	}
 
 	if token != "" {
-		record, err := h.validateExchangeApproval(ctx, id, token)
-		if err != nil {
-			return nil, err
-		}
+		return h.approveRIExchangeViaToken(ctx, id, token)
+	}
 
-		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.approveRIExchangeViaSession(ctx, req, id, nil)
+}
 
-		return h.executeApprovedExchange(ctx, id, record)
+// 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
 	}
 
-	return h.approveRIExchangeViaSession(ctx, req, id, nil)
+	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)
 }
 
 // approveRIExchangeViaSession is the session-authed branch of approveRIExchange

internal/api/handler_ri_exchange_test.go

@@ -353,7 +353,8 @@ func TestApproveRIExchange_SessionAdmin(t *testing.T) {
 	req := &events.LambdaFunctionURLRequest{
 		Headers: map[string]string{"authorization": "Bearer admin-bearer"},
 	}
-	_, _ = h.approveRIExchange(ctx, req, id, "")
+	_, 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)
@@ -403,7 +404,8 @@ func TestApproveRIExchange_SessionApproveOwn(t *testing.T) {
 		req := &events.LambdaFunctionURLRequest{
 			Headers: map[string]string{"authorization": "Bearer owner-bearer"},
 		}
-		_, _ = h.approveRIExchange(ctx, req, id, "")
+		_, err := h.approveRIExchange(ctx, req, id, "")
+		require.NoError(t, err)
 		mockStore.AssertCalled(t, "TransitionRIExchangeStatus", ctx, id, "pending", "processing")
 	})
 
@@ -463,7 +465,8 @@ func TestApproveRIExchange_LegacyTokenStillWorks(t *testing.T) {
 	mockStore.On("FailRIExchange", ctx, id, mock.AnythingOfType("string")).Return(nil)
 
 	req := &events.LambdaFunctionURLRequest{}
-	_, _ = h.approveRIExchange(ctx, req, id, token)
+	_, err := h.approveRIExchange(ctx, req, id, token)
+	require.NoError(t, err)
 	mockStore.AssertCalled(t, "TransitionRIExchangeStatus", ctx, id, "pending", "processing")
 }